celestia-node icon indicating copy to clipboard operation
celestia-node copied to clipboard

enable govet fieldalignment

Open ramin opened this issue 2 years ago • 5 comments

digging through old issues found the request for integrating "structslop" with ci. The tool of which does some struct memory alignment checks. I was surprised this wasn't already present in golangci-lint and voila they do have some of these checks as part of govet

  • this enabled the fieldalignment govet check
  • at current run, we have a handlful (30 or so) suggestions, which i think is quite noisy but yes, probably decent optimizations

@Wondertan i think this satisfies the original request in the old issue, if you want to proceed, maybe we impart the fixes as part of this PR too

fixes #893

ramin avatar Oct 18 '23 10:10 ramin

current warnings

header/header.go:36:21: fieldalignment: struct with 360 pointer bytes could be 344 (govet)
type ExtendedHeader struct {
                    ^
header/header.go:241:23: fieldalignment: struct with 56 pointer bytes could be 40 (govet)
	return json.Marshal(&struct {
	                     ^
header/header.go:256:10: fieldalignment: struct with 56 pointer bytes could be 40 (govet)
	aux := &struct {
	        ^
nodebuilder/node/admin.go:15:13: fieldalignment: struct with 24 pointer bytes could be 16 (govet)
type module struct {
            ^
nodebuilder/node/admin.go:29:11: fieldalignment: struct with 16 pointer bytes could be 8 (govet)
type Info struct {
          ^
nodebuilder/fraud/lifecycle.go:23:53: fieldalignment: struct with 88 pointer bytes could be 80 (govet)
type ServiceBreaker[S service, H libhead.Header[H]] struct {
                                                    ^
share/p2p/params.go:11:17: fieldalignment: struct with 40 pointer bytes could be 8 (govet)
type Parameters struct {
                ^
api/rpc/server.go:22:13: fieldalignment: struct with 56 pointer bytes could be 48 (govet)
type Server struct {
            ^
libs/edssser/edssser.go:20:13: fieldalignment: struct with 32 pointer bytes could be 8 (govet)
type Config struct {
            ^
libs/edssser/edssser.go:30:14: fieldalignment: struct with 104 pointer bytes could be 64 (govet)
type EDSsser struct {
             ^
nodebuilder/blob/cmd/blob.go:162:15: fieldalignment: struct with 16 pointer bytes could be 8 (govet)
		response := struct {
		            ^
nodebuilder/blob/cmd/blob.go:205:16: fieldalignment: struct with 56 pointer bytes could be 48 (govet)
	type tempBlob struct {
	              ^
nodebuilder/p2p/cmd/p2p.go:274:11: fieldalignment: struct with 40 pointer bytes could be 24 (govet)
			return struct {
			       ^
nodebuilder/p2p/cmd/p2p.go:313:11: fieldalignment: struct with 40 pointer bytes could be 24 (govet)
			return struct {
			       ^
nodebuilder/p2p/cmd/p2p.go:381:11: fieldalignment: struct with 40 pointer bytes could be 24 (govet)
			return struct {
			       ^
nodebuilder/p2p/cmd/p2p.go:421:11: fieldalignment: struct with 40 pointer bytes could be 24 (govet)
			return struct {
			       ^
nodebuilder/share/cmd/share.go:60:11: fieldalignment: struct with 48 pointer bytes could be 24 (govet)
			return struct {
			       ^
header/headertest/fraud/testing.go:27:17: fieldalignment: struct with 56 pointer bytes could be 48 (govet)
type FraudMaker struct {
                ^
nodebuilder/tests/swamp/swamp.go:51:12: fieldalignment: struct with 536 pointer bytes could be 512 (govet)
type Swamp struct {
           ^
api/rpc_test.go:139:16: fieldalignment: struct with 16 pointer bytes could be 8 (govet)
	var tests = []struct {
	              ^
blob/blob.go:67:11: fieldalignment: struct with 88 pointer bytes could be 80 (govet)
type Blob struct {
          ^
blob/blob.go:111:15: fieldalignment: struct with 64 pointer bytes could be 56 (govet)
type jsonBlob struct {
              ^
blob/blob_test.go:22:15: fieldalignment: struct with 24 pointer bytes could be 16 (govet)
	var test = []struct {
	             ^
blob/service_test.go:50:15: fieldalignment: struct with 32 pointer bytes could be 24 (govet)
	var test = []struct {
	             ^
core/listener.go:32:15: fieldalignment: struct with 64 pointer bytes could be 56 (govet)
type Listener struct {
              ^
das/checkpoint.go:7:17: fieldalignment: struct with 32 pointer bytes could be 16 (govet)
type checkpoint struct {
                ^
das/checkpoint.go:17:23: fieldalignment: struct with 24 pointer bytes could be 8 (govet)
type workerCheckpoint struct {
                      ^
das/coordinator.go:15:26: fieldalignment: struct with 224 pointer bytes could be 192 (govet)
type samplingCoordinator struct {
                         ^
das/daser.go:24:12: fieldalignment: struct with 192 pointer bytes could be 152 (govet)
type DASer struct {
           ^
das/done.go:8:11: fieldalignment: struct with 24 pointer bytes could be 16 (govet)
type done struct {
          ^
das/state.go:12:23: fieldalignment: struct with 104 pointer bytes could be 40 (govet)
type coordinatorState struct {
                      ^
das/state.go:43:19: fieldalignment: struct with 32 pointer bytes could be 24 (govet)
type retryAttempt struct {
                  ^
das/stats.go:4:20: fieldalignment: struct with 40 pointer bytes could be 16 (govet)
type SamplingStats struct {
                   ^
das/stats.go:24:18: fieldalignment: struct with 48 pointer bytes could be 24 (govet)
type WorkerStats struct {
                 ^
das/worker.go:22:13: fieldalignment: struct with 128 pointer bytes could be 112 (govet)
type worker struct {
            ^
das/worker.go:43:10: fieldalignment: struct with 48 pointer bytes could be 16 (govet)
type job struct {
         ^
das/backoff_test.go:16:13: fieldalignment: struct with 48 pointer bytes could be 24 (govet)
	tests := []struct {
	           ^
das/backoff_test.go:50:13: fieldalignment: struct with 128 pointer bytes could be 112 (govet)
	tests := []struct {
	           ^
das/coordinator_test.go:333:18: fieldalignment: struct with 88 pointer bytes could be 56 (govet)
type mockSampler struct {
                 ^
das/coordinator_test.go:446:17: fieldalignment: struct with 16 pointer bytes could be 8 (govet)
type checkOrder struct {
                ^
das/coordinator_test.go:520:11: fieldalignment: struct with 16 pointer bytes could be 8 (govet)
type lock struct {
          ^
das/daser_test.go:305:17: fieldalignment: struct with 40 pointer bytes could be 24 (govet)
type mockGetter struct {
                ^
header/headertest/testing.go:31:16: fieldalignment: struct with 56 pointer bytes could be 32 (govet)
type TestSuite struct {
               ^
libs/keystore/fs_keystore.go:18:17: fieldalignment: struct with 32 pointer bytes could be 24 (govet)
type fsKeystore struct {
                ^
libs/keystore/map_keystore.go:14:18: fieldalignment: struct with 32 pointer bytes could be 24 (govet)
type mapKeystore struct {
                 ^
nodebuilder/config.go:27:13: fieldalignment: struct with 640 pointer bytes could be 608 (govet)
type Config struct {
            ^
nodebuilder/node.go:45:11: fieldalignment: struct with 296 pointer bytes could be 272 (govet)
type Node struct {
          ^
nodebuilder/store.go:143:14: fieldalignment: struct with 64 pointer bytes could be 48 (govet)
type fsStore struct {
             ^
nodebuilder/header/config.go:23:13: fieldalignment: struct with 232 pointer bytes could be 184 (govet)
type Config struct {
            ^
nodebuilder/p2p/bitswap.go:66:20: fieldalignment: struct with 64 pointer bytes could be 56 (govet)
type bitSwapParams struct {

ramin avatar Oct 18 '23 10:10 ramin

funnily enough @Wondertan, the first PR i made @distractedm1nd said "oh, lint is failing on some alignment issue" and i thought it was this check and thought "damn, that is cool to run that as a CI check", and well...here we are.

ramin avatar Oct 18 '23 10:10 ramin

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (150378f) 50.99% compared to head (6bb4018) 50.79%.

Files Patch % Lines
share/eds/byzantine/share_proof.go 50.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2856      +/-   ##
==========================================
- Coverage   50.99%   50.79%   -0.20%     
==========================================
  Files         176      176              
  Lines       11172    11172              
==========================================
- Hits         5697     5675      -22     
- Misses       4974     4993      +19     
- Partials      501      504       +3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Nov 20 '23 12:11 codecov-commenter

i had a nightmare about this open PR so came and figured i'd come and do all the alignment work so its simple and ready to go for you @Wondertan

noisy yes, but its exactly 69 files changed so "noice" too 😆

I found a good tool betteralignment that would make suggested alignment/padding fixes and preserve comments etc so we don't lose readability, i set it to ignore test files too as it would particularly flip out on named table driven tests.

ramin avatar Nov 20 '23 12:11 ramin

Would you want to make a similar PR to go-header?

Wondertan avatar Nov 21 '23 21:11 Wondertan

this one has lingered, is pretty monumental, disruptive, and the overall experience of both the linter pointing out these vet/field alignment issues and then trying to fix them kinda sucks. Rather than blanket requiring field alignment EVERYWHERE then marking as //nolint, we should align structs where performance is critical then we can add a test for those that are as and when

ramin avatar Jun 12 '24 10:06 ramin