Skip to content
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

enable govet fieldalignment #2856

Closed
wants to merge 19 commits into from
Closed

enable govet fieldalignment #2856

wants to merge 19 commits into from

Conversation

ramin
Copy link
Contributor

@ramin ramin commented Oct 18, 2023

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
Copy link
Contributor Author

ramin commented Oct 18, 2023

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
Copy link
Contributor Author

ramin commented Oct 18, 2023

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 ramin added the kind:ci CI related PRs label Oct 19, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2023

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 ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ramin
Copy link
Contributor Author

ramin commented Nov 20, 2023

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.

Wondertan
Wondertan previously approved these changes Nov 21, 2023
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your first massive PR ❤️
They grow so fast 🥲 😄

@Wondertan
Copy link
Member

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

vgonkivs
vgonkivs previously approved these changes Jan 9, 2024
distractedm1nd
distractedm1nd previously approved these changes Jan 22, 2024
Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is config breaking, since it changes the serialization order in nodebuilder/config.go. Can you verify and mark it as such?

ramin added 4 commits March 8, 2024 10:23
…core/listener.go,header/headertest/testing.go,nodebuilder/blobl/cmd/blob.go and share/p2p/peers/manager.go
@ramin ramin dismissed stale reviews from distractedm1nd and vgonkivs via d6713d8 March 8, 2024 10:43
@ramin
Copy link
Contributor Author

ramin commented Jun 12, 2024

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 ramin closed this Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:ci CI related PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lint: Add structslop linter to golangci-linter
5 participants