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

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

kkoreilly
Copy link
Contributor

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
Copy link

CLAassistant commented Jul 6, 2024

CLA assistant check
All committers have signed the CLA.

@rcoreilly
Copy link
Contributor

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 mvertes added enhancement New feature or request area/extract wrapper generation labels Jul 17, 2024
@mvertes
Copy link
Member

mvertes commented Jul 17, 2024

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
Copy link
Contributor Author

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.

Copy link
Member

@mvertes mvertes left a comment

Choose a reason for hiding this comment

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

Ok, let's give a try to dependencies.

@mvertes mvertes added this to the v0.16.x milestone Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/extract wrapper generation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants