From e00ee60ba60e006a3dcd01465668874411f3d5ac Mon Sep 17 00:00:00 2001 From: Mark Spicer Date: Wed, 7 Jun 2023 10:44:32 -0400 Subject: [PATCH] feat: Add profile to pixlet check. (#811) This change adds profiling to the pixlet check command. In addition, it adds a render check given it needs to be able to render before we can profile an app. Finally, I've made improvements to output silencing so the pixlet check output is clear. --- cmd/check.go | 43 +++++++++++++++++++++-------- cmd/community/loadapp.go | 10 ++++++- cmd/community/validateicons.go | 10 ++++++- cmd/profile.go | 50 ++++++++++++++++++++++------------ cmd/render.go | 20 +++++++------- runtime/applet.go | 6 +++- 6 files changed, 97 insertions(+), 42 deletions(-) diff --git a/cmd/check.go b/cmd/check.go index 7e27b6b8f3..675d00b37c 100644 --- a/cmd/check.go +++ b/cmd/check.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "strings" + "time" "github.com/bazelbuild/buildtools/buildifier/utils" "github.com/fatih/color" @@ -13,6 +14,8 @@ import ( "tidbyt.dev/pixlet/manifest" ) +const MaxRenderTime = 500000000 // 500ms + func init() { CheckCmd.Flags().BoolVarP(&rflag, "recursive", "r", false, "find apps recursively") } @@ -60,16 +63,6 @@ func checkCmd(cmd *cobra.Command, args []string) error { continue } - // Create temporary file for app rendering. - f, err := os.CreateTemp("", "") - if err != nil { - return fmt.Errorf("could not create temp file for rendering, check your system: %w", err) - } - defer os.Remove(f.Name()) - - // TODO: Check if app will render once we are able to enable target - // determination. - // Check if an app can load. err = community.LoadApp(cmd, []string{app}) if err != nil { @@ -121,8 +114,34 @@ func checkCmd(cmd *cobra.Command, args []string) error { failure(app, fmt.Errorf("manifest contains spelling errors: %w", err), fmt.Sprintf("try `pixlet community spell-check --fix %s`", manifestFile)) continue } - // TODO: enable spell check for apps once we can run it successfully - // against the community repo. + + // Create temporary file for app rendering. + f, err := os.CreateTemp("", "") + if err != nil { + return fmt.Errorf("could not create temp file for rendering, check your system: %w", err) + } + defer os.Remove(f.Name()) + + // Check if app renders. + silenceOutput = true + output = f.Name() + err = render(cmd, []string{app}) + if err != nil { + foundIssue = true + failure(app, fmt.Errorf("app failed to render: %w", err), "try `pixlet render` and resolve any runtime issues") + continue + } + + // Check performance. + p, err := ProfileApp(app, map[string]string{}) + if err != nil { + return fmt.Errorf("could not profile app: %w", err) + } + if p.DurationNanos > MaxRenderTime { + foundIssue = true + failure(app, fmt.Errorf("app takes too long to render %s", time.Duration(p.DurationNanos)), fmt.Sprintf("try optimizing your app using `pixlet profile %s` to get it under %s", app, time.Duration(MaxRenderTime))) + continue + } // If we're here, the app and manifest are good to go! success(app) diff --git a/cmd/community/loadapp.go b/cmd/community/loadapp.go index a2658d0c39..1714c64d70 100644 --- a/cmd/community/loadapp.go +++ b/cmd/community/loadapp.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/spf13/cobra" + "go.starlark.net/starlark" "tidbyt.dev/pixlet/runtime" ) @@ -34,8 +35,15 @@ func LoadApp(cmd *cobra.Command, args []string) error { runtime.InitHTTP(cache) runtime.InitCache(cache) + // Remove the print function from the starlark thread. + initializers := []runtime.ThreadInitializer{} + initializers = append(initializers, func(thread *starlark.Thread) *starlark.Thread { + thread.Print = func(thread *starlark.Thread, msg string) {} + return thread + }) + applet := runtime.Applet{} - err = applet.Load(script, src, nil) + err = applet.LoadWithInitializers(script, src, nil, initializers...) if err != nil { return fmt.Errorf("failed to load applet: %w", err) } diff --git a/cmd/community/validateicons.go b/cmd/community/validateicons.go index f2ab4592ed..d20bd470c0 100644 --- a/cmd/community/validateicons.go +++ b/cmd/community/validateicons.go @@ -7,6 +7,7 @@ import ( "os" "github.com/spf13/cobra" + "go.starlark.net/starlark" "tidbyt.dev/pixlet/icons" "tidbyt.dev/pixlet/runtime" "tidbyt.dev/pixlet/schema" @@ -38,8 +39,15 @@ func ValidateIcons(cmd *cobra.Command, args []string) error { runtime.InitHTTP(cache) runtime.InitCache(cache) + // Remove the print function from the starlark thread. + initializers := []runtime.ThreadInitializer{} + initializers = append(initializers, func(thread *starlark.Thread) *starlark.Thread { + thread.Print = func(thread *starlark.Thread, msg string) {} + return thread + }) + applet := runtime.Applet{} - err = applet.Load(args[0], src, nil) + err = applet.LoadWithInitializers(args[0], src, nil, initializers...) if err != nil { return fmt.Errorf("failed to load applet: %w", err) } diff --git a/cmd/profile.go b/cmd/profile.go index 33b12d0d1c..1199366847 100644 --- a/cmd/profile.go +++ b/cmd/profile.go @@ -80,48 +80,64 @@ func profile(cmd *cobra.Command, args []string) error { config[split[0]] = split[1] } + profile, err := ProfileApp(script, config) + if err != nil { + return err + } + + options := &pprof_driver.Options{ + Fetch: MakeFetchFunc(profile), + UI: printUI{}, + } + if err = pprof_driver.PProf(options); err != nil { + return fmt.Errorf("could not start pprof driver: %w", err) + } + + return nil +} + +func ProfileApp(script string, config map[string]string) (*pprof_profile.Profile, error) { src, err := ioutil.ReadFile(script) if err != nil { - return fmt.Errorf("failed to read file %s: %w", script, err) + return nil, fmt.Errorf("failed to read file %s: %w", script, err) } cache := runtime.NewInMemoryCache() runtime.InitHTTP(cache) runtime.InitCache(cache) + // Remove the print function from the starlark thread. + initializers := []runtime.ThreadInitializer{} + initializers = append(initializers, func(thread *starlark.Thread) *starlark.Thread { + thread.Print = func(thread *starlark.Thread, msg string) {} + return thread + }) + applet := runtime.Applet{} - err = applet.Load(script, src, nil) + err = applet.LoadWithInitializers(script, src, nil, initializers...) if err != nil { - return fmt.Errorf("failed to load applet: %w", err) + return nil, fmt.Errorf("failed to load applet: %w", err) } buf := new(bytes.Buffer) if err = starlark.StartProfile(buf); err != nil { - return fmt.Errorf("error starting profiler: %w", err) + return nil, fmt.Errorf("error starting profiler: %w", err) } - _, err = applet.Run(config) + _, err = applet.Run(config, initializers...) if err != nil { _ = starlark.StopProfile() - return fmt.Errorf("error running script: %w", err) + return nil, fmt.Errorf("error running script: %w", err) } if err = starlark.StopProfile(); err != nil { - return fmt.Errorf("error stopping profiler: %w", err) + return nil, fmt.Errorf("error stopping profiler: %w", err) } profile, err := pprof_profile.ParseData(buf.Bytes()) if err != nil { - return fmt.Errorf("could not parse pprof profile: %w", err) - } - - options := &pprof_driver.Options{ - Fetch: MakeFetchFunc(profile), - UI: printUI{}, - } - if err = pprof_driver.PProf(options); err != nil { - return fmt.Errorf("could not start pprof driver: %w", err) + return nil, fmt.Errorf("could not parse pprof profile: %w", err) } - return nil + return profile, nil } diff --git a/cmd/render.go b/cmd/render.go index 6da1257fd1..03779c4e9c 100644 --- a/cmd/render.go +++ b/cmd/render.go @@ -100,16 +100,6 @@ func render(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to read file %s: %w", script, err) } - cache := runtime.NewInMemoryCache() - runtime.InitHTTP(cache) - runtime.InitCache(cache) - - applet := runtime.Applet{} - err = applet.Load(script, src, nil) - if err != nil { - return fmt.Errorf("failed to load applet: %w", err) - } - // Remove the print function from the starlark thread if the silent flag is // passed. initializers := []runtime.ThreadInitializer{} @@ -120,6 +110,16 @@ func render(cmd *cobra.Command, args []string) error { }) } + cache := runtime.NewInMemoryCache() + runtime.InitHTTP(cache) + runtime.InitCache(cache) + + applet := runtime.Applet{} + err = applet.LoadWithInitializers(script, src, nil, initializers...) + if err != nil { + return fmt.Errorf("failed to load applet: %w", err) + } + roots, err := applet.Run(config, initializers...) if err != nil { return fmt.Errorf("error running script: %w", err) diff --git a/runtime/applet.go b/runtime/applet.go index a2dedcea9d..6a78aa8566 100644 --- a/runtime/applet.go +++ b/runtime/applet.go @@ -92,6 +92,10 @@ func (a *Applet) thread(initializers ...ThreadInitializer) *starlark.Thread { // and the actual code should be passed in src. Optionally also pass // loader to make additional starlark modules available to the script. func (a *Applet) Load(filename string, src []byte, loader ModuleLoader) (err error) { + return a.LoadWithInitializers(filename, src, loader) +} + +func (a *Applet) LoadWithInitializers(filename string, src []byte, loader ModuleLoader, initializers ...ThreadInitializer) (err error) { defer func() { if r := recover(); r != nil { err = fmt.Errorf("panic while executing %s: %v", a.Filename, r) @@ -116,7 +120,7 @@ func (a *Applet) Load(filename string, src []byte, loader ModuleLoader) (err err "struct": starlark.NewBuiltin("struct", starlarkstruct.Make), } - globals, err := starlark.ExecFile(a.thread(), a.Filename, a.src, a.predeclared) + globals, err := starlark.ExecFile(a.thread(initializers...), a.Filename, a.src, a.predeclared) if err != nil { return fmt.Errorf("starlark.ExecFile: %v", err) }