traefik / yaegi

Yaegi is Another Elegant Go Interpreter
https://pkg.go.dev/github.com/traefik/yaegi
Apache License 2.0
6.94k stars 343 forks source link

Use x/tools/go/packages to significantly improve speed of extract code generation #1642

Open kkoreilly opened 2 months ago

kkoreilly commented 2 months ago

This PR updates extract to use golang.org/x/tools/go/packages instead of go/importer when possible, which results in an 11x speedup for the first time and a 45x speedup for future extraction due to caching.

This is the raw timing output for running yaegi extract on a collection of various packages of different sizes:

Old:

~/go/src/cogentcore.org/core/yaegicore/symbols/ > time ./make
cosh run succeeded
      155.80 real       212.81 user       152.78 sys

New:

~/go/src/cogentcore.org/core/yaegicore/symbols/ > time ./make
cosh run succeeded
       13.78 real        27.16 user        17.79 sys
~/go/src/cogentcore.org/core/yaegicore/symbols/ > time ./make
cosh run succeeded
        3.43 real         5.13 user        16.74 sys
CLAassistant commented 2 months ago

CLA assistant check
All committers have signed the CLA.

rcoreilly commented 1 month ago

we are not sure why the generated code tests are failing -- they don't fail when we run locally.

Also, this PR is included in #1647 so could be closed depending on your preference for how to proceed.

mvertes commented 1 month ago

The performance improvement is nice, congrats for that.

But it comes at the cost of adding new dependencies, other than Go standard library, something that we have avoided to do so far for this project. The mishandling of dependency may be the cause of the CI failure, but I have not yet analyzed it in details.

The improvement concerns the extract command, which is rarely executed, only at generate stage, and doesn't translate into speed gains for the interpreter, so I do not consider this as high priority.

I do not want to close completely the door. We could consider incorporating this kind of improvement if there is a way to isolate the extract command in its own dependency set, with a separate go.mod, and making sure that the interpreter package's go.mod remains as it is. It induces multiple go.mod for the project, which I'm not a big fan of, but the trade-off may be interesting. For example, to be able to add other dependencies to the CLI executable (but not the interpreter package itself), and add readline improvements such as history, completion, etc.

kkoreilly commented 1 month ago

The inclusion of a dependency outside of the standard library may be unideal, but it has no concrete impact on the interpreter. Go is designed such that packages are only included in the binary and the go.mod if they are actually used, so any downstream package using the interpreter will not be burdened by the dependency. The inclusion of a couple extra lines in the go.mod file will not harm anyone.

Moreover, the dependencies that this adds are from golang.org/x, which is still part of the Go Project and maintained by the Go Team. These are not unstable packages of questionable quality; these are reasonable extensions of the standard library developed by the same developers as the standard library itself. As you can see here, x/tools/go/packages is widely used by many Go packages, including by Google itself.

Finally, this is not a low-priority, rarely executed thing for us. As part of our three projects that use yaegi extensively (a live GUI playground, a command-line shell, and an interactive data science platform), we entirely depend on including pre-compiled GUI, shell, and data science packages in the interpreter. As we develop these projects, we frequently make changes and additions that require re-generating the extract code, and it is extremely disruptive for that process to take minutes when everything else is practically instant.

We appreciate the invaluable functionality provided by yaegi, and we hope that you can prioritize providing a powerful and efficient platform over avoiding any dependencies.