Skip to content

Reduce allocations in app server watcher#61985

Merged
rosstimothy merged 1 commit intomasterfrom
tross/app_watcher
Dec 4, 2025
Merged

Reduce allocations in app server watcher#61985
rosstimothy merged 1 commit intomasterfrom
tross/app_watcher

Conversation

@rosstimothy
Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy commented Dec 4, 2025

The slice of resources received from the watcher contains a copy of all the items known by the watcher. The app server then cloned each app via types.Application.Copy to get a handle to a types.AppV3 so that the public address could be set if it was empty. The cloned apps are then added to a new slice which is provided to the reconciler.

To reduce memory consumption, types.Application.SetPublicAddr was added to make it possible to set the address without copying an app. The slice from the resource watcher is now reused instead of allocating a new slice for the amended applications.

changelog: Reduced memory consumption of the Application service.

select {
case apps := <-watcher.ResourcesC:
appsWithAddr := make(types.Apps, 0, len(apps))
appsWithAddr := apps[:0]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is there a reason why you don't change apps slice in place?
nothing here skips the append to appsWithAddr, so you can just range the apps and set the public address without having to append anything or am I missing something?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call. I've updated this in af6aa44 and updated the watcher tests to validate the public address is set accordingly.

@rosstimothy rosstimothy requested a review from tigrato December 4, 2025 11:57
Copy link
Copy Markdown
Contributor

@nicholasmarais1158 nicholasmarais1158 left a comment

Choose a reason for hiding this comment

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

Code looks good.

The slice of resources receivied from the watcher contains a copy
of all the items known by the watcher. The app server then cloned
each app via types.Application.Copy to get a handle to a types.AppV3
so that the public address could be set if it was empty. The cloned
apps are then added to a _new_ slice which is provided to the reconciler.

To reduce memory consumption, types.Application.SetPublicAddr was added
to make it possible to set the address without copying an app. The slice
from the resource watcher is now reused instead of allocating a new slice
for the amended applications.
@rosstimothy rosstimothy enabled auto-merge December 4, 2025 12:56
@rosstimothy rosstimothy added this pull request to the merge queue Dec 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 4, 2025
@rosstimothy rosstimothy added this pull request to the merge queue Dec 4, 2025
Merged via the queue into master with commit 8d78cc8 Dec 4, 2025
43 checks passed
@rosstimothy rosstimothy deleted the tross/app_watcher branch December 4, 2025 20:00
@backport-bot-workflows
Copy link
Copy Markdown
Contributor

@rosstimothy See the table below for backport results.

Branch Result
branch/v17 Create PR
branch/v18 Create PR

21KennethTran pushed a commit that referenced this pull request Jan 6, 2026
The slice of resources receivied from the watcher contains a copy
of all the items known by the watcher. The app server then cloned
each app via types.Application.Copy to get a handle to a types.AppV3
so that the public address could be set if it was empty. The cloned
apps are then added to a _new_ slice which is provided to the reconciler.

To reduce memory consumption, types.Application.SetPublicAddr was added
to make it possible to set the address without copying an app. The slice
from the resource watcher is now reused instead of allocating a new slice
for the amended applications.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants