-
Notifications
You must be signed in to change notification settings - Fork 18k
cmd/go: completion of lazy loading [freeze exception] #42288
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
Comments
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 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. |
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. |
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. |
(#36460 remains the canonical issue for tracking lazy loading work.) |
Change https://golang.org/cl/275172 mentions this issue: |
…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 <[email protected]> Run-TryBot: Bryan C. Mills <[email protected]> Reviewed-by: Jay Conrod <[email protected]> TryBot-Result: Go Bot <[email protected]>
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 forcmd/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 bycmd/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 thego.mod
file, so even if we were to wait for the 1.17 cycle it would not trigger even for most users running ago
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 allowreplace
directives, and one common use-case forreplace
directives is to prune out invalid versions required by irrelevant transitive dependencies. With lazy loading, fewerreplace
directives will be needed, so fewer authors will need to work around the limitations ofgo install pkg@version
.-mod=readonly
will require users to rungo get
andgo mod tidy
in more situations. We have been ironing out bugs fromgo 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:
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
The text was updated successfully, but these errors were encountered: