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

SeederThreadPoolExecutor seemingly ignoring "maxPoolSize" setting and is stuck on 16 thread maximum #1220

Open
mbosecke opened this issue Feb 17, 2024 · 6 comments

Comments

@mbosecke
Copy link

No matter how many seed tasks I submit to GWC, it only will ever run 16 at a time. After looking at the code it seems like SeederThreadPoolExecutor is configured with a corePoolSize of 16 and a maxPoolSize of 32, however, it's also using an underlying "LinkedBlockingQueue" to store queued tasks and the docs say:

Using an unbounded queue (for example a LinkedBlockingQueue without a predefined capacity) will cause new tasks to wait in the queue when all corePoolSize threads are busy. Thus, no more than corePoolSize threads will ever be created. (And the value of the maximumPoolSize therefore doesn't have any effect.)

Effectively, the queue is infinitely large, therefore it never fills up and therefore the ThreadPoolExecutor never adds threads into the pool above and beyond the original 16.

(Bonus: it would also be a nice to expose a way for end-users to change the min/max size of the thread pool)

@mbosecke
Copy link
Author

Proposed solution:

  • Continue using an unbounded LinkedBlockingQueue so that end-users can submit as many tasks as they wish.
  • Do not differentiate between "corePoolSize" and "maxPoolSize" within the code since only the former is relevant. Perhaps refer to them as one "maxNumberOfThreads" variable that is used for both arguments.
  • Consider setting "allowCoreThreadTimeout(true)" on the ThreadPoolExecutor so the number of idle threads will reduce down to 0 when there are no active seeding tasks. I'm not confident on this point.
  • Expose "maxNumberOfThreads" to the end-user as a configurable option.

Some ideas can be found in how the built-in "Executors" java class constructs various specialised thread pools.

@mbosecke
Copy link
Author

To provide some context on my use case, I have a machine with 80 cores that I'd like to leverage to seed the hell out of some layers.

@aaime
Copy link
Member

aaime commented Feb 19, 2024

@mbosecke at first I was surprised by it turns out that indeed, you're right, when executing "submit" the code creates a new thread only if there are less than "corePoolSize", otherwise tries to submit to the queue first, and only if this one refuses, then an attempt to go beyond is made (with eventual failure if the max pool size is reached).

Given seeds typically happen in bursts, I agree with your proposal, probably better to set core and max at the same value, and have a timeout and let the threads die when unused after some time (e.g., one minute?).

Given that SeederThreadLocalTransferExecutor is used only in GeoWebCache, I'd suggest creating a no-argument constructor, that uses a sane default, and can be controlled via system/environment/servlet context variable:

 private static final int POOL_SIZE;

    static {
        POOL_SIZE =
                Optional.ofNullable(GeoWebCacheExtensions.getProperty("gwc.seeder.pool.size"))
                        .map(Integer::parseInt)
                        .orElse(Runtime.getRuntime().availableProcessors() * 2);
    }

    public SeederThreadPoolExecutor() {
        super(POOL_SIZE, POOL_SIZE, 60, TimeUnit.SECONDS, new LinkedBlockingQueue<>(), tf);
        allowCoreThreadTimeOut(true);
    }

The creation of the pool in the Spring app context would use such default constructor. The SeederThreadLocalTransferExecutor would be modified accordingly, and also use the default constructor in the Spring context.

@smithkm @groldan opinions?

@mbosecke
Copy link
Author

In the case where GWC is being used within geoserver, does this thread pool get implicitly used during normal operations (ex a WMS request comes in and a metatile gets cached)? If so, I'd make an argument that we may want to keep the thread pool primed with some idle threads somehow. However, I suspect that's not the case, and it's only used when someone is explicitly seeding, in which case I think it's reasonably for it to dwindle itself down to 0 threads over the course of a minute.

@aaime
Copy link
Member

aaime commented Feb 20, 2024

Yes, ideally I'd love to have the pool stay at corePoolSize, expand up to the max one under load, and start adding to the queue only after all the workers are busy. But I don't see how to get that, the pool seems to offer stuff to the queue first, and only expand the set of workers later, which does not seem that useful to me.

@aaime
Copy link
Member

aaime commented May 13, 2024

It seems nobody has so far found a good way out. Given this, I'd be ok setting the minimum pool size equal to the maximum to better leverage machines with many cores. I've seen as a common practice to spin-up a powerful, but short lived machine to perform seed jobs on a specific layer, this issue is basically at odds with it, the dedicated hardware won't be used properly, if there are more than 8 cores (typical peak throughput point is concurrent requests at 2-4 times the number of cores).

@smithkm @groldan thoughts?

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

No branches or pull requests

2 participants