Skip to content

cache: includes cpu features in cache keys #2220

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

Merged
merged 3 commits into from
May 28, 2024
Merged

Conversation

mathetake
Copy link
Member

there's a discussion about "offline" compilation, and if that happens,
the CPU feature incompatibility becomes the problem.
This hardens the cache against it by including the cpu id in the file cache keys
so that the content will be used exactly on the same CPUs.

Note that this impacts only amd64 target.

there's a discussion about "offline" compilation,
and if that happens, the CPU feature incompatibility
becomes the problem.

This hardens the cache against it by including the cpu
id in the file cache keys so that the content will be
used exactly on the same CPUs.

Note that this impacts only amd64 target.

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake mathetake requested review from anuraaga and ncruces May 28, 2024 01:52
@mathetake mathetake marked this pull request as ready for review May 28, 2024 01:56
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake mathetake requested a review from evacchi May 28, 2024 02:19
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake mathetake requested a review from anuraaga May 28, 2024 08:31
@mathetake
Copy link
Member Author

thanks guys!

@mathetake mathetake merged commit d4a4903 into main May 28, 2024
64 checks passed
@mathetake mathetake deleted the cpufeaturesincachekey branch May 28, 2024 08:44
@ncruces
Copy link
Contributor

ncruces commented May 28, 2024

I'm a bit ambivalent about this one. OTOH, this avoids crashing with is Good™. OTOH this may actually make it harder to support “offline” caching.

For offline caching, which I don't know if we want to support, IMO the best approach would be a flag that tells the compiler not to use any features above the Go baseline (i.e. assume no CPU flags) and then you can safely Go embed the compiled thing, if you favor startup speed over huge binaries. Including CPU flags in the cache key might make it harder to do that.

So I guess the usefulness of this, as is, is whether people are OK with configuring a file cache, then filling it up in CI, and then distributing the thing with (IKD?) Docker or something. It's not my world, so I have no idea if this is what people are looking for.

That said, this is done, and works, and is safer, and doesn't break anything, so I'm good. Pick your battles and stuff.

👍

@anuraaga
Copy link
Contributor

@ncruces Thanks for mentioning the Go baseline - had the random thought that perhaps we shouldn't do any cupid detection. Since Go has GOAMD64, I think it is also a build tag. Maybe it's more "pure-Go" to just use the build tags instead of detection, wazero code is no better than Go code?

Anyways, I don't think it's an important, or even great, enhancement to implement, but maybe an idea for where there is a will and a want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants