-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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/compile: uwriter output is nondeterministic #69547
Comments
There are two changes in the diff:
The second change, in the last 8 bytes of the payload (before the final $$), is just the fingerprint of the whole file, which is expected to change if there is any other change in the data. So, the only significant change is the first one. If we include in in the previous unchanged byte (03) we notice that it is a reordering of two three-byte strings (03 d4 03) and (03 d8 04). This suggests a nondeterministic map iteration in the encoder. So what are these bytes? Instrumenting the ureader, the pos value (base offset) is 301ec, which means the file-relative offset 863b2 of the reordered string is the section-relative offset 863b2 - 301ec = 561c6. Dumping all the elemEnds, we find that this one falls within the range of elemEnds[12207]:
However, a panic statement added to the decoding of that element (for type information only, by x/tools/internal/gcimporter/main.go) is never executed, so I suspect it is an element needed only by the compiler (e.g. escape, inlining, generics) and not part of the type information. I may have made a mistake in my analysis above, but I think this is a problem for the compiler team. |
Change https://go.dev/cl/614717 mentions this issue: |
@sluongng Do you have a chance to check whether this problem happen with go1.22.x? |
I briefly rolled back my repo to 1.22.7 to do the same test but the diff is much bigger
Here are the 2 Not sure what to make of it. I have yet to run any of the gcexports tools on them to see if I can make sense of the diff somehow. |
If I understand correctly, you first compiled "db.go" with version ABC of a dependency Looking through the files of the build files on bazel-contrib/rules_go#4111 (comment) . There are gorm '.x' files that are different. First I found is
I suspect this is WAI. I think the first thing to do is to examine the build cache after building |
Correct.
That would be very disappointing to know. We intentionally use |
In haste I completely missed the part where you (@sluongng) mentioned that you upgraded a dependency between the two builds. @timothy-king is right: there is more information in an export data file than just type information. Even the same type information can be (deterministically) written in multiple ways that will cause build cache misses. The the Go tools (compiler, linker, etc) are intended to be fully deterministic pure functions (ignoring the special case of post-link executable stamping), and the The key test would be to delete the output files (e.g. go/blaze clean), to re-execute the build (without any changes to the inputs), and to check whether it re-creates an identical output file. If so, then there is no determinism problem. |
Hmm that's surprising and disappointing to hear. Our main use case in rules_go was to divide compile outputs into It sounds like we will only benefit from (a) and not (b). My question is: do you think our use case makes sense to warrant a feature request here? Or is it something the Go team is not willing to support? Side note: I would love to read more about the gc export data information if there are any documents available. I tried parsing through |
Dividing compile outputs into export information and object code is a very worthwhile optimization; we do it in Blaze (the version of Blaze used internally at Google), and analogous things in other languages (for example, Java's JVM class files contain both type information and function bodies, and only the former is needed for downstream compilation, so it makes sense to split the files). I confess I thought Bazel's rules_go already did this. Nonetheless, the issue @timothy-king is describing is that the exported information used by downstream compilation is more than meets the eye. It contains not just the types of public declarations; it also holds information used by escape analysis, inlining, and generics, and all of these can be very sensitive to small changes in indirect dependencies. So it will change even when the types proper do not. But it is still a pure function of the transitive closure of dependencies---not of the previous states.
What exactly are you asking for? I think it might be instructive if there were a way to inspect every byte of the export data file so that you could understand how it depends on the inputs. But I think the toolchain already does everything you actually need.
There isn't a whole lot of documentation on it besides these two items:
but I did an internal presentation a while back that doesn't contain any confidential information, and though it's quite rough, you might find it useful. I have attached a PDF below. |
I am going to close this as I am not sure there is anything we can do. If you have a more specific feature request, please file a new issue. You may also want to ask questions in the #compilers channel of the Gophers slack.
The .x files for dependencies should still be fine to cache on for a Bazel/Blaze setup. These will change less often, but they may still change. These files are off by a few bytes. You may end up with .x cache hits fairly often if the information in the namespaces does not change. It may be interesting to measure this at some point.
You may end up downloading 2 copies of a lot of the information most compilations with such a strategy. Both copies would need coherent descriptions of many [most] of the types. For really big go programs, what I have seen is that the types can be much larger than the other data, and these need to be in both parts, and even more importantly are often required transitively in multiple packages. (Think big generated protos that are passed around.) This is the kind of thing you have to measure to know if it helps, and it may not help uniformly.
Yes it is. The documentation is lacking and we have recently lost some institutional knowledge. The best documentation is the source for the writer at the moment: https://github.com/golang/go/blob/master/src/cmd/compile/internal/noder/noder.go . That is even slower going, but it is the most accurate. |
Thank you both for the great insights. This is valuable information for us to improve Bazel's rules_go further. 🙇 Let me do some reading and jump on Gophers Slack with more questions. |
Go version
1.23.0 (Linux AMD64 and MacOS ARM64)
Output of
go env
in your module/workspace:What did you do?
Context
https://github.com/bazelbuild/rules_go is an alternative way to build Go application using Bazel. It does so by manually instrumenting the go tools such as
go tool compile
orgo tool link
in hermetic Bazel sandboxes. The intermediate artifacts is then cached by Bazel for subsequent reuse.What I did
For the Compile step, we are currently calling
go tool compile
with both-o <compile-output>.x
and-linkobj <link-output>.a
. The.x
file is then used for subsequent downstream package compilations while the.a
file will be used for downstream executable linkings.Here is the relevant code: https://github.com/bazelbuild/rules_go/blob/8b8294b6359a9c0a89af968fb7092f6e7194f420/go/tools/builders/compilepkg.go#L494-L495
What did you see happen?
When I upgraded
gorm.io/driver/mysql
, a direct dependency ofdb.go
, to a newer version, I noticed that the newdb.x
was changed.The binary diff between 2
.x
files are just a few bytes smallI tried using different tools I could find in
x/tools
to better understand the content of the file but it seems like the content is pretty consistent between 2 filesI also used a modified version of
x/tools/internal/gcimporter/main.go
to analyze the fileBut still cannot see any diff between the 2 files.
Here are the 2 files attached.
db.tar.gz
What did you expect to see?
The expectation here is that if we were to compile package
github.com/buildbuddy-io/buildbuddy/server/util/db/db.go
, if there were no changes in exported data indb.go
, thendb.x
will have reproducible content, and thus, downstream packages which depend ondb.x
will not need to recompile.The text was updated successfully, but these errors were encountered: