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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

field alignment for generated structs #1117

Open
1 task done
napei opened this issue Jan 2, 2021 · 11 comments
Open
1 task done

field alignment for generated structs #1117

napei opened this issue Jan 2, 2021 · 11 comments
Labels

Comments

@napei
Copy link
Contributor

napei commented Jan 2, 2021

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 馃挕

Because of how memory allocation is done in go, it's possible to optimise the amount of memory that a struct takes by ordering fields based on their size. In the case of structs inside ./ent there is possibly some performance/memory benefits by ordering these during codegen.

Consider this struct (comments removed):

type Address struct { // 176 pointer bytes
	config     `json:"-"`
	ID         uuid.UUID    `json:"id"`
	CreateTime time.Time    `json:"create_time,omitempty"`
	UpdateTime time.Time    `json:"update_time,omitempty"`
	Line1      string       `json:"line1"`
	Suburb     string       `json:"suburb"`
	State      string       `json:"state"`
	Postcode   string       `json:"postcode"`
	Edges      AddressEdges `json:"edges"`
}
type Address struct { // 160 pointer bytes
	config     `json:"-"`
	CreateTime time.Time    `json:"create_time,omitempty"`
	UpdateTime time.Time    `json:"update_time,omitempty"`
	Postcode   string       `json:"postcode"`
	Line1      string       `json:"line1"`
	Suburb     string       `json:"suburb"`
	State      string       `json:"state"`
	Edges      AddressEdges `json:"edges"`
	ID         uuid.UUID    `json:"id"`
}

Not exactly a dramatic change, but quite possible could be a helpful change with more complex structs especially when part of the usage of ent is to load data into memory and do stuff with it.

Because it's an internal package inside golang, it's hard to use programmatically, instead needing to be made into a cli tool..perhaps the algorithms can be taken and re-used internally in ent.

package main

import (
	"golang.org/x/tools/go/analysis/multichecker"
	"golang.org/x/tools/go/analysis/passes/fieldalignment"
)

func main() {
	multichecker.Main(fieldalignment.Analyzer)
}
Example: running on an ent directory
$ align -fieldalignment ./ent

/home/napei/crvc-backend/ent/address.go:16:14: struct with 176 pointer bytes could be 160
/home/napei/crvc-backend/ent/address_delete.go:17:20: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/address_query.go:22:19: struct with 152 pointer bytes could be 136
/home/napei/crvc-backend/ent/address_query.go:479:21: struct with 104 pointer bytes could be 88
/home/napei/crvc-backend/ent/address_update.go:19:20: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/address_update.go:275:23: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/allergy.go:17:14: struct with 168 pointer bytes could be 136
/home/napei/crvc-backend/ent/allergy_delete.go:17:20: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/allergy_query.go:21:19: struct with 160 pointer bytes could be 136
/home/napei/crvc-backend/ent/allergy_query.go:482:21: struct with 104 pointer bytes could be 88
/home/napei/crvc-backend/ent/allergy_update.go:19:20: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/allergy_update.go:252:23: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/config.go:14:13: struct with 40 pointer bytes could be 32
/home/napei/crvc-backend/ent/ent.go:124:22: struct with 32 pointer bytes could be 24
/home/napei/crvc-backend/ent/ent.go:216:22: struct with 32 pointer bytes could be 24
/home/napei/crvc-backend/ent/flag.go:16:11: struct with 160 pointer bytes could be 144
/home/napei/crvc-backend/ent/flag_delete.go:17:17: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/flag_query.go:22:16: struct with 152 pointer bytes could be 136
/home/napei/crvc-backend/ent/flag_query.go:479:18: struct with 104 pointer bytes could be 88
/home/napei/crvc-backend/ent/flag_update.go:19:17: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/flag_update.go:262:20: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/mutation.go:41:22: struct with 168 pointer bytes could be 152
/home/napei/crvc-backend/ent/mutation.go:708:22: struct with 152 pointer bytes could be 136
/home/napei/crvc-backend/ent/mutation.go:1319:19: struct with 160 pointer bytes could be 144
/home/napei/crvc-backend/ent/mutation.go:1931:20: struct of size 216 could be 208
/home/napei/crvc-backend/ent/mutation.go:2821:18: struct of size 240 could be 232
/home/napei/crvc-backend/ent/mutation.go:3882:21: struct of size 192 could be 176
/home/napei/crvc-backend/ent/owner.go:18:12: struct with 256 pointer bytes could be 232
/home/napei/crvc-backend/ent/owner_delete.go:17:18: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/owner_query.go:22:17: struct with 168 pointer bytes could be 144
/home/napei/crvc-backend/ent/owner_query.go:544:19: struct with 104 pointer bytes could be 88
/home/napei/crvc-backend/ent/owner_update.go:20:18: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/owner_update.go:381:21: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/pet.go:17:10: struct with 280 pointer bytes could be 248
/home/napei/crvc-backend/ent/pet.go:50:15: struct with 32 pointer bytes could be 16
/home/napei/crvc-backend/ent/pet_delete.go:17:16: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/pet_query.go:23:15: struct with 168 pointer bytes could be 144
/home/napei/crvc-backend/ent/pet_query.go:549:17: struct with 104 pointer bytes could be 88
/home/napei/crvc-backend/ent/pet_update.go:21:16: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/pet_update.go:478:19: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/record.go:17:13: struct with 176 pointer bytes could be 152
/home/napei/crvc-backend/ent/record.go:32:18: struct with 56 pointer bytes could be 40
/home/napei/crvc-backend/ent/record_delete.go:17:19: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/record_query.go:24:18: struct with 176 pointer bytes could be 152
/home/napei/crvc-backend/ent/record_query.go:615:20: struct with 104 pointer bytes could be 88
/home/napei/crvc-backend/ent/record_update.go:21:19: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/record_update.go:375:22: struct with 72 pointer bytes could be 56
/home/napei/crvc-backend/ent/tx.go:13:9: struct with 184 pointer bytes could be 144

It's an extra checker for gopls that I only recently turned on and has fixed a few things for me so far.

@napei napei changed the title field alignment for structs field alignment for generated structs Jan 2, 2021
@a8m
Copy link
Member

a8m commented Jan 5, 2021

Thanks for this proposal @napei.

I find it cleaner to provide this functionality using a standalone CLI tool that can be executed with another go generate rule. In this case, it will also work if additional fields will be added to the different structs with external templates.

Is there any CLI that uses fieldalignment and provides this reformatting?

@a8m a8m added the Proposal label Jan 5, 2021
@napei
Copy link
Contributor Author

napei commented Jan 5, 2021

Is there any CLI that uses fieldalignment and provides this reformatting?

This code generates a CLI application which can run the analysis on a file. The only thing it doesn't do well is preserving comments.

Unfortunately fieldalignment is an internal package, or at least the ability to manually run analysers programmatically is internal and a pain.

@komuw
Copy link

komuw commented Nov 2, 2021

https://github.com/orijtech/structslop possibly can

structslop --help
  -apply
    	apply suggested fixes (using -fix won't work)

@frederikhors
Copy link
Contributor

frederikhors commented Jun 13, 2022

Launching fieldalignment at the end of generation is a problem because it obviously changes the order of some structs generating issues like this:

image

In this case err is needed before err.Error() now.

And as @napei said it also deletes some comments (the ones on structs it aligns).

@vtolstov
Copy link

Comments are problem, but issues like you provide is not a problem. I think that generated code must use struct field names

@a8m
Copy link
Member

a8m commented Jun 14, 2022

Comments are problem, but issues like you provide is not a problem. I think that generated code must use struct field names

Agree. Feel free to send a patch or I can do it later. Thanks 馃檹

@giautm
Copy link
Collaborator

giautm commented Jun 14, 2022

Sent a patch: #2648

@frederikhors
Copy link
Contributor

Can we check if there are PRs on golang repository for fieldalignment removing comments? Where to look?

@giautm
Copy link
Collaborator

giautm commented Jun 14, 2022

I tested the PR with my project, now the fieldalignment can run success without any issue.

@frederikhors
Copy link
Contributor

I tested the PR with my project, now the fieldalignment can run success without any issue.

Except for comments, right?

What do you think about align struct before applying comments during generation steps?

@giautm
Copy link
Collaborator

giautm commented Jun 14, 2022

I tested the PR with my project, now the fieldalignment can run success without any issue.

Except for comments, right?

What do you think about align struct before applying comments during generation steps?

Yes, fieldalignment remove all comments on the struct fields. The comment and code was generate by Go template as GQLGen/other tools did. Do you have any idea to align the struct with Go template?

But, imo the comment should keep for public field only, because the private field was common to grouped by the intent - which may not in the right order after struct align.

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

No branches or pull requests

6 participants