Improve performance of state tool #1467
Conversation
| @@ -162,7 +162,7 @@ func run() (rerr error) { | |||
| mReload := mProjects.AddSubMenuItem("Reload", "Reload the local projects listing") | |||
| localProjectsUpdater := menu.NewLocalProjectsUpdater(mProjects) | |||
|
|
|||
| localProjects, err := model.LocalProjects() | |||
| localProjects, err := model.LocalProjects(context.Background()) | |||
There was a problem hiding this comment.
It looks like you use context.Background() everywhere. Is that correct? I assume that you have experimented with time-out contexts but determined that they are not necessary anymore.
I am fine if we leave this commit (814c6ed) as-is anyways as it gives us the flexibility to introduce timeouts here. But I would like to ensure that I understand it correctly that it does not change any behavior right now.
There was a problem hiding this comment.
This is just preserving the behavior that was there prior to this PR. I just made it more explicit.
Some of these could certainly benefit from a timeout, but they were not the focus of this PR.
|
|
||
| baseUrl := fmt.Sprintf("http://127.0.0.1:%d", port) | ||
| return &Client{ | ||
| Client: gqlclient.NewWithOpts(fmt.Sprintf("%s/query", baseUrl), 0, graphql.WithHTTPClient(http.DefaultClient)), | ||
| // The custom client bypasses http-retry, which we don't need for doing local requests |
| svcManager := svcmanager.New(cfg) | ||
| require.NoError(t, svcManager.Start()) | ||
|
|
||
| ctx, _ := context.WithTimeout(context.Background(), 5*time.Second) |
There was a problem hiding this comment.
Ah, I see. It is used for testing. :)
There was a problem hiding this comment.
Yeah we're not really using this particular context atm, the refactor was mainly so that I could use the context for NewSvcModel() appropriately. In the previous version we were reusing it for all client requests which will almost certainly eventually trigger a timeout in a way that it shouldn't.
| var profilingEnabled = false | ||
|
|
||
| func init() { | ||
| profilingEnabled = os.Getenv(constants.ProfileEnvVarName) == "true" |
There was a problem hiding this comment.
nit: We do not need an init() function for this initialization.
There was a problem hiding this comment.
I could swear Go didn't let you call functions outside of a function.. not sure why this works.
… slower than mac (at least on CI)
Each commit addresses a different issue, so it might be easier to review this commit by commit.