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

Update system/status structs #70

Merged
merged 2 commits into from
Oct 7, 2022
Merged

Update system/status structs #70

merged 2 commits into from
Oct 7, 2022

Conversation

davidnewhall
Copy link
Contributor

Closes #67.

Updates and alphabetizes structs for system/status.

Copy link
Contributor

@Fuochi Fuochi left a comment

Choose a reason for hiding this comment

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

just 2 notes:
1- please think about start writing tests, they'll save a lot of time
2- why alphabetic order for struct fields? lately, I've been thinking that we could try to lower the memory usage by sorting using fieldalignment or something similar. what do you think?

@davidnewhall
Copy link
Contributor Author

Alphabetical seems like it makes it easy to identify changes. But I also agree it's irrational given how Go stores structures in memory.

I'll start putting tests in for any new methods.

@davidnewhall
Copy link
Contributor Author

I played with this for a bit. Does not seem to matter what order I put the fields in, the Lidarr struct is always 328 bytes. Try it: https://go.dev/play/p/4zrxs5qm8Ir

Maybe t's just that struct, but I'm not seeing an easy win here. :(

@Fuochi
Copy link
Contributor

Fuochi commented Oct 7, 2022

I played with this for a bit. Does not seem to matter what order I put the fields in, the Lidarr struct is always 328 bytes. Try it: https://go.dev/play/p/4zrxs5qm8Ir

Maybe t's just that struct, but I'm not seeing an easy win here. :(

yes, since all fields but bool are multiple of 8 bytes, and there are 9 bool, the only way to have a bigger struct is to have 3 or more groups of bools (https://go.dev/play/p/d18Fu0jxx2d) I think that putting time.Time at first and string to follow should optimize a bit the runtime usage, but honestly, I didn't fully get how... more findings here: 1pkg/gopium#24 (comment)

However, if there really is an improvement, we're talking about minimal. Hence, feel free do do as you prefer.
The only real benefit that comes to my mind: If we use fieldalignment -fix and let it decide the order of the fields, we'll have consistency across the code, no matter who writes it.

@davidnewhall
Copy link
Contributor Author

davidnewhall commented Oct 7, 2022

Can you link me to the fieldalignment tool you mentioned?
EDIT:
This one?

go install golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment

EDIT2:
Weird..

src/golift.io/starr$ fieldalignment -v golift.io/starr/radarr
fieldalignment: internal error: package "bytes" without types was imported from "golift.io/starr/radarr"

src/golift.io/starr$ fieldalignment -v golift.io/starr/prowlarr
fieldalignment: internal error: package "strings" without types was imported from "golift.io/starr/prowlarr"

src/golift.io/starr$ fieldalignment -v golift.io/starr/starrcmd
fieldalignment: internal error: package "fmt" without types was imported from "golift.io/starr/starrcmd"

@Fuochi
Copy link
Contributor

Fuochi commented Oct 7, 2022

sure, package is: https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/fieldalignment
usage:

go install golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment@latest
fieldalignment -fix ./

it could be enabled as linter as well as part of govet in golangci:

linters-settings:
  govet:
    enable:
      - fieldalignment

@Fuochi
Copy link
Contributor

Fuochi commented Oct 7, 2022

btw, it's really a small thing, don't waste too much time here

@davidnewhall
Copy link
Contributor Author

lidarr/system.go:15:19: fieldalignment: struct with 320 pointer bytes could be 296 (govet)
type SystemStatus struct {

@davidnewhall
Copy link
Contributor Author

I can only get this to work through golangci-lint, and not by running it directly I suspect I have a library out of date, but not sure how to fix it.

@davidnewhall
Copy link
Contributor Author

I'm just going to worry about optimizing the structs later; there's hundreds that need fixing, and I can't get this dang tool to work right.

@davidnewhall davidnewhall merged commit 0cf700e into master Oct 7, 2022
@davidnewhall davidnewhall deleted the dn2_updates branch October 7, 2022 07:12
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.

Update System/Status to pull all fields in api
2 participants