Skip to content

Conversation

SentryMan
Copy link
Collaborator

This lib is only meant to be used with virtual threads, what need is there to customize the executor?

@SentryMan SentryMan requested a review from rbygrave March 18, 2025 03:34
@SentryMan SentryMan self-assigned this Mar 18, 2025
@SentryMan SentryMan enabled auto-merge (squash) March 18, 2025 04:48
@rbygrave
Copy link
Contributor

rbygrave commented Mar 18, 2025

what need is there to customize the executor?

For running performance comparison testing between Virtual Threads and Platform threads?
To provide some custom Executor that might have a maximum limit or some other interesting feature?

I know Helidon SE had effectively their own executor to provide nice fast graceful shutdown - graceful shutdown that doesn't rely on AtomicLong counters for active requests (I designed the first cut of that).

I'd be loathed to remove this just yet [with the native synchronise fix in 23, not yet in a LTS version / 25]

This lib is only meant to be used with virtual threads, what need is there to customize the executor?
@SentryMan
Copy link
Collaborator Author

For running performance comparison testing between Virtual Threads and Platform threads?

Some features in this server (like sse) have been designed with the assumption of virtual threads. We also have no async support, so I believe it to be a bit unwise to use platform threads.

To provide some custom Executor that might have a maximum limit or some other interesting feature?

If someone needs it, we can re-add it. As is though, I think it's a potential landmine.

[with the native synchronise fix in 23]

The fix is in JDK 24, but I don't see what that has to do with anything, it's not like they'll backport it to 21. I've spent the last month upgrading my job's CI/CD to work with JDK 24 so I'm unbothered.

In any case, without async you'll end up blocking a platform thread no matter if you use VT/PT with synchronized.

@rbygrave
Copy link
Contributor

ok, let merge it.

@SentryMan SentryMan merged commit 47c2829 into avaje:master Mar 18, 2025
4 checks passed
@SentryMan SentryMan deleted the remove-executor branch March 18, 2025 18:52
@robaho
Copy link

robaho commented Mar 19, 2025

Often times depending on work load a pool of platform threads will outperform virtual threads. Yes, they may all be consumed and so latency can go up if the server is saturated but modern OS easily handle 10k threads. Which is often enough for many systems - especially if special use.

@SentryMan
Copy link
Collaborator Author

Well alright then

SentryMan added a commit that referenced this pull request Mar 19, 2025
@robaho
Copy link

robaho commented Mar 19, 2025

I had a version of robaho httpserver that used a platform executor and passed off a runnable to handle the connection once some data was available to read. It was surprisingly efficient, but as I said, it often depends on the workload.

The bigger reason I would suggest leaving it is that people might be in a monitoring executor - gathered stats, etc - it would be a transparent way of doing this.

But, I don't know the api profile here - was simply pointing out an alternative view.

@SentryMan
Copy link
Collaborator Author

simply pointing out an alternative view.

A very convincing alternative view

rbygrave pushed a commit that referenced this pull request Mar 19, 2025
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