Skip to content

GUACAMOLE-2316: Allow the number of display worker threads per connection to be limited. - #703

Open
msshapira wants to merge 2 commits into
apache:mainfrom
msshapira:GUACAMOLE-2316
Open

GUACAMOLE-2316: Allow the number of display worker threads per connection to be limited.#703
msshapira wants to merge 2 commits into
apache:mainfrom
msshapira:GUACAMOLE-2316

Conversation

@msshapira

Copy link
Copy Markdown

Adds a per-connection limit on the number of encoder worker threads created for each guac_display, configurable via max_worker_threads in guacd.conf, -w on the command line, and MAX_WORKER_THREADS in the Docker image.

The limit is stored on guac_client (appended to the struct) and read by guac_display_alloc(), so it transports from guacd across the protocol-plugin boundary without changing any existing public signature. Default 0 preserves current behavior.

See GUACAMOLE-2316 for the motivation and the open question about whether the default should stay 0/unlimited or become a finite value.

@necouchman necouchman left a comment

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.

Overall I'm fine with the changes; however, I wonder if there's something a bit different we should do for the default maximum of threads? Maybe the default of scaling to the maximum number of processors isn't the best option? Is there some sort of default maximum that's actually more appropriate for performance - that is, over X number of threads, you don't see any additional benefit?

@msshapira

Copy link
Copy Markdown
Author

Overall I'm fine with the changes; however, I wonder if there's something a bit different we should do for the default maximum of threads? Maybe the default of scaling to the maximum number of processors isn't the best option? Is there some sort of default maximum that's actually more appropriate for performance - that is, over X number of threads, you don't see any additional benefit?

@necouchman I agree that a hard limit may be a good idea. Opus suggested X=4, assuming more that that won't give much benefit anyway. Agree?

@necouchman

Copy link
Copy Markdown
Contributor

Overall I'm fine with the changes; however, I wonder if there's something a bit different we should do for the default maximum of threads? Maybe the default of scaling to the maximum number of processors isn't the best option? Is there some sort of default maximum that's actually more appropriate for performance - that is, over X number of threads, you don't see any additional benefit?

@necouchman I agree that a hard limit may be a good idea. Opus suggested X=4, assuming more that that won't give much benefit anyway. Agree?

I'll defer to @mike-jumper on that one - he probably has a better idea of the cross-over point at which the number of threads will become more of a burden than it is a help.

@mike-jumper

Copy link
Copy Markdown
Contributor

I think the default should remain as-is. There's plenty of parallelism to exploit in the render pipeline, even more so with WIP performance improvements to guac_display, and adding an arbitrary default ceiling would hobble performance on servers with the CPU to spare.

In my testing of the display, I've seen improvement in restricting the display to P-cores on platforms where the distinction exists, but a reduction in available workers only harms performance beyond that, especially since the FIFO ensures that the usage of the worker pool scales to the workload. Some workers may sit idle, but that's harmless vs. not having them available.

Comment thread src/libguac/display.c Outdated
Comment on lines +154 to +155
* own set of worker threads), and a single display will rarely produce
* enough parallelizable work to occupy a large number of processors. On

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.

This is only true for very low levels of activity and relatively low resolutions, and only important if running under unusually strict resource levels. It's more important to have the threads available in case they are needed or users will hit a performance wall.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed the comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@mike-jumper can you approve this?

msshapira and others added 2 commits August 7, 2026 09: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.

3 participants