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

cmd/go: completion of lazy loading [freeze exception] #42288

Closed
bcmills opened this issue Oct 30, 2020 · 5 comments
Closed

cmd/go: completion of lazy loading [freeze exception] #42288

bcmills opened this issue Oct 30, 2020 · 5 comments
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Oct 30, 2020

I've been putting in as many hours as I reasonably can over the last week trying to get lazy module loading (#36460) ready before the Nov. 1 freeze. I've made a lot of progress, but there's no way I'll realistically have the capstone CL ready to mail before the freeze, and even if it were ready it isn't obvious that reviewers in the U.S. would have the bandwidth to complete a thorough code review by the end of next week.

I'd like to request a freeze exception to continue work on in for an extra week or two beyond the freeze. Here's my reasoning.

On the “pro” side:

Continuing work for a week or two adds relatively little risk to the release, and mitigates some other risks.

The feedback we receive on modules changes in cmd/go during the freeze is, unfortunately, minimal.

For various reasons, the Google-internal regression testing of the release throughout the cycle by and large does not cover cmd/go or the module system. While the period from the freeze to the beta is a rich source of feedback about the compiler, standard library, and runtime, it provides essentially no additional confidence for cmd/go changes. (As evidence: consider the many regressions that went unreported during the Go 1.13 freeze — even up through the release candidates! — such as: #34747, #34679, #34497, #34215.)

Contributors to the Go project do use cmd/go regularly throughout the cycle, but they only rarely update dependencies, and the Go project's dependency graphs — especially the dependencies of the standard library — are quite small and hygienic and do not exhibit the kinds of cases that would uncover subtle bugs in the module loader, nor the cases where lazy loading is likely to make much of a difference in user experience.

Moreover, cmd/go is much more deterministic than many of the other parts of the distribution (particularly the runtime), so we are unlikely to benefit very much from additional soak time in the Go project builders. (Regressions caught by cmd/go tests tend to break builders consistently — they do not generally manifest as low-frequency flakes that would benefit from many repetitions.)

Finally, lazy loading is gated on the go version specified in the go.mod file, so even if we were to wait for the 1.17 cycle it would not trigger even for most users running a go command built from head.

We have intentionally front-loaded the integration-testing for #36460.

Having learned my lesson about the lack of pre-release feedback in Go 1.13, I spent the early part of the 1.16 cycle adding extensive regression and integration tests for the planned lazy-loading feature work, such as those in CL 222341. Those tests are derived from the lazy loading design doc and intended to be easy to adapt when lazy loading is ready, so that we can have reasonable test coverage for lazy loading as soon as the implementation is complete. (Even without lazy loading, the “control cases” for the new tests incidentally revealed many existing bugs in the go command, which I then fixed as part of the refactoring leading up to lazy loading — part of the reason it's been taking so long.)

Getting lazy loading into the 1.16 release mitigates risks from other modules changes.

We have already merged module retraction (#24031), go install pkg@version (#40276), and defaulting to -mod=readonly (#40728). All of those features benefit substantially from also having lazy loading.

  • Module retraction will report fewer retracted modules when irrelevant dependencies are pruned.

  • go install outside of a module can be much more efficient if it does not need to resolve potentially-large chains of irrelevant dependencies. In particular, go install pkg@version intentionally does not allow replace directives, and one common use-case for replace directives is to prune out invalid versions required by irrelevant transitive dependencies. With lazy loading, fewer replace directives will be needed, so fewer authors will need to work around the limitations of go install pkg@version.

  • -mod=readonly will require users to run go get and go mod tidy in more situations. We have been ironing out bugs from go get (particularly cmd/go: go get module/pkg@master doesn't seem to work sometimes #37438 / [CL 263267]), but those commands will be less likely to stumble over any other existing bugs — and will hopefully execute much more efficiently — if they do not have to deal with large, irrelevant transitive dependency graphs.

We are also reasonably likely to encounter interesting interactions between -mod=readonly, go get, go install, and lazy loading at some point after the first beta in which they are all present. If that necessitates changes to any of those other features, it would be nice to be able to make those changes holistically instead of working within the parameters of behaviors cemented into the final 1.16 release without accompanying lazy-loading feedback.

The lazy-loading changes will be relatively easy to roll back if needed.

Having the final changes for lazy loading land very late in the cycle would mean that very little else is built on top of them. If for some reason we ended up needing to roll them back, we wouldn't have to disentangle a lot of intervening changes.

(The only other UX change so far for lazy loading is the narrowing of the all package pattern, which is already flag-gated and IMO is a positive change regardless of lazy loading proper.)


On the “con” side:

The refactoring needed for lazy loading is difficult to flag-gate

Lazy loading requires that we replace the flat modload.buildList variable with a more structured graph. I am not comfortable writing the change in a way that maintains the flat list in parallel with the graph: there would be a high risk of skew between the graph and list implementations, and most of the existing tests would only exercise one representation or the other, substantially reducing the effective code coverage of our new and existing tests.

It will be relatively easy to gate off the actual lazy-loading behavior using the existing constants, but there isn't much we can do to gate the effects of refactoring proper without losing vital code coverage and/or introducing even more risk due to more-complex-than-necessary code.

Continuing development somewhat eats in to our documentation bandwidth.

We know that the module documentation still needs work (#33637 and others), and lazy loading is another use-case that will require documentation. The longer we spend implementing it, the higher the risk that we will under-document it.


Taking the above into account, I suggest that we:

  • Continue development of lazy loading for two weeks into the freeze, then reassess progress and risks with an eye toward not delaying the planned December 1 beta.
  • Keep the existing change to the all package pattern, but rename its flag-constant if the rest of lazy-loading proper does not make the cut.

CC @jayconrod @matloob @rsc @golang/release

@bcmills bcmills added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. GoCommand cmd/go release-blocker modules labels Oct 30, 2020
@bcmills bcmills added this to the Go1.16 milestone Oct 30, 2020
@mvdan
Copy link
Member

mvdan commented Oct 30, 2020

Contributors to the Go project do use cmd/go regularly throughout the cycle, but they only rarely update dependencies

For what it's worth, I'd be happy to spend an hour or two doing extra tests with lazy loading (e.g. update to go 1.17 in go.mod, update dependencies, tidy, etc) once it lands. If you write a comment or post outlining the main things you want Go users to try and play with, I could also share that to get more eyes during the beta.

I think, historically, the lack of the above is why most non-contributors don't try modules changes until they're final. Unless you closely follow the Go project, you wouldn't even know that there are important modules changes that would benefit from testing. It's something @Lyoness and I have talked about in the past.

@dmitshur
Copy link
Contributor

dmitshur commented Nov 6, 2020

We've discussed this request in a release meeting, and I see @rsc has reacted with 👍 as well. The rationale looks good.

Approved.

Please do reassess progress and be mindful of time as we work towards the beta release, and update your plan if you think a change in approach becomes needed.

Thank you @mvdan for offering to assist with early testing.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Nov 6, 2020
@bcmills
Copy link
Contributor Author

bcmills commented Dec 1, 2020

There is still a fair bit of work (and a lot of testing and test cleanup) left to do, and we're now at the nominal Beta 1 release date (Dec. 1). After discussion with @rsc, @jayconrod, and @matloob, in the interest of not holding up the beta we've decided to push lazy loading to early 1.17.

@bcmills bcmills closed this as completed Dec 1, 2020
@bcmills
Copy link
Contributor Author

bcmills commented Dec 1, 2020

(#36460 remains the canonical issue for tracking lazy loading work.)

@gopherbot
Copy link

Change https://golang.org/cl/275172 mentions this issue: cmd/go/internal/modload: rename constants to reflect that lazy loading is not yet implemented

gopherbot pushed a commit that referenced this issue Dec 3, 2020
…g is not yet implemented

Updates #36460
Updates #42288

Change-Id: I82a3b7e97a8e2f83bae2318ca9fb5c38c0c811cd
Reviewed-on: https://go-review.googlesource.com/c/go/+/275172
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Dec 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

4 participants