Add field alignment

[hawksight]

Hey @lukaszraczylo thank you for taking the time to submit a PR here.

Could you just explain the motive for this, is it purely a syntax / tidy up of the code in this PR? There are no functional changes in this?

[lukaszraczylo]

Hi @hawksight, I spoke about it with @davidcollom privately. The purpose of the field alignment in Go is to optimise memory usage. Visualisation on how it works in practice can be seen on the attached gif :)

Bit of read if you're interested - https://levelup.gitconnected.com/struct-optimization-a-small-change-for-a-huge-impact-1f816c783014

[davidcollom]

Hey @lukaszraczylo Thanks for raising this, would you be able to provide us with some rough benchmarking values or before/after so that we can see how much we're likely to save?

[ribbybibby]

If this turns out to be a worthwhile optimisation, should we add some sort of CI tooling/linter to ensure this happens for new/edited structs?

[lukaszraczylo]

Hi @ribbybibby / @davidcollom, There are no benchmarks as I'd need to write the code for it ( which I unfortunately don't have time for at the moment ).

Ps. There's another PR from me - with significant improvement of the pipeline execution time. In general, it's much faster to build the cross-platform Go binaries outside of the container and inject them into the container ( of the appropriate arch ) than building them within the container, which, for example, slows things down massively in the arm64 case.

[davidcollom]

I took a look at betteralign and found the following from prior to this PR:

~/repos/github/jetstack/version-checker (main!$)$ betteralign ./...
/Users/david.collom/repos/github/jetstack/version-checker/pkg/api/types.go:44:14: 8 bytes saved: struct of size 64 could be 56
/Users/david.collom/repos/github/jetstack/version-checker/pkg/client/acr/acr.go:25:13: 16 bytes saved: struct with 72 pointer bytes could be 56
/Users/david.collom/repos/github/jetstack/version-checker/pkg/client/ecr/ecr.go:16:13: 8 bytes saved: struct with 360 pointer bytes could be 352
/Users/david.collom/repos/github/jetstack/version-checker/pkg/client/gcr/gcr.go:32:19: 8 bytes saved: struct with 32 pointer bytes could be 24
/Users/david.collom/repos/github/jetstack/version-checker/pkg/client/ghcr/ghcr.go:18:13: 8 bytes saved: struct with 32 pointer bytes could be 24
/Users/david.collom/repos/github/jetstack/version-checker/pkg/client/quay/pager.go:12:12: 24 bytes saved: struct with 96 pointer bytes could be 72
/Users/david.collom/repos/github/jetstack/version-checker/pkg/client/quay/quay.go:43:23: 8 bytes saved: struct with 24 pointer bytes could be 16
/Users/david.collom/repos/github/jetstack/version-checker/pkg/client/selfhosted/selfhosted.go:37:14: 8 bytes saved: struct with 96 pointer bytes could be 88
/Users/david.collom/repos/github/jetstack/version-checker/pkg/client/client.go:41:13: 16 bytes saved: struct with 40 pointer bytes could be 24
/Users/david.collom/repos/github/jetstack/version-checker/pkg/client/client.go:47:14: 8 bytes saved: struct with 216 pointer bytes could be 208
/Users/david.collom/repos/github/jetstack/version-checker/pkg/cache/cache.go:14:12: 32 bytes saved: struct with 64 pointer bytes could be 32
/Users/david.collom/repos/github/jetstack/version-checker/pkg/cache/cache.go:26:16: 8 bytes saved: struct with 48 pointer bytes could be 40
/Users/david.collom/repos/github/jetstack/version-checker/pkg/version/semver/semver.go:14:13: 24 bytes saved: struct with 48 pointer bytes could be 24
/Users/david.collom/repos/github/jetstack/version-checker/pkg/controller/checker/checker.go:20:13: 8 bytes saved: struct with 48 pointer bytes could be 40
/Users/david.collom/repos/github/jetstack/version-checker/cmd/app/options.go:64:14: 24 bytes saved: struct with 376 pointer bytes could be 352
/Users/david.collom/repos/github/jetstack/version-checker/cmd/app/options.go:274:24: 8 bytes saved: struct with 24 pointer bytes could be 16

Along with an after (this branch):

~/repos/github/jetstack/version-checker (field-alignment!$)$ betteralign ./...
/Users/david.collom/repos/github/jetstack/version-checker/cmd/app/options.go:64:14: 8 bytes saved: struct with 352 pointer bytes could be 344

There's not a huge improvement in my opinion - but as @ribbybibby and I raised, if we could implement something like betteralign to keep track of this, I'd be happier to accept/merge this

[aidy]

I'm not entirely against this, but I do think that this is an optimisation at the cost of code legibility.

For example, here: https://github.com/jetstack/version-checker/pull/192/files#diff-0be7332a6934801efa8562d347d9cf7829eabdc6b85795cb68dd5696bf7edc5cR396-R401

I would always expect that the exp* parameters would be next to each other, and probably at the bottom of the struct. Having the test parameters and expectations be intermingled rather than clearly set out reduces legibility and increases the maintenance burden.

I was curious why this wasn't just a compiler optimisation, and found this: https://github.com/golang/go/issues/10014#issuecomment-91436342 - tldr; memory optimisation may come at the cost of an increase in cache misses.

On the whole, without benchmarks (of both memory and runtime) to support this change, I'd argue that the optimisation here isn't worth the cost of reduced legibility.

[github-actions[bot]]

This Pull Request is stale because it has been open for 60 days with no activity. It will be closed in 7 days if no further activity.