Add stickyAffinity() to Thread.Builder.OfVirtual#226
Conversation
Virtual threads with sticky affinity preserve carrier locality: - start/unpark from a sticky VT uses lazy submission (local queue, no signal) - sub-pollers (mode 2 and 3) are made sticky, simplifying Poller.polled()
|
👋 Welcome back franz1981! A progress list of the required criteria for merging this PR into |
|
@franz1981 This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 1 new commit pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. ➡️ To flag this PR as ready for integration with the above commit message, type |
Webrevs
|
|
Using https://github.com/franz1981/Netty-VirtualThread-Scheduler/blob/master/PERFORMANCE.md#io-bound-configuration-max-tps-8-cores with
Performance (I/O-bound: 8 cores, 30ms mock, 10K connections, NIO, per-request VT): and per-op perf stat counters: Which shows that there's a massive reduction in context switches and better IPC. At 120K tps instead, way below saturation: |
|
Can you confirm that "FJP reference" is the current scheduler, it's just that Thread.yield is changed to use lazySubmit unconditionally when scheduling the tasks for the poller threads. Is "Both commits" your custom scheduler that it tied to Netty? |
So both are FJP. |
|
Okay, would it be possible to clarify this in the first table? I think it would help to collapse the rows for the commits into one so that we have a simple picture of what is bring compared. (I think the main point Thread.yield using lazySubmit is the difference and that this can be achieved with a small API change, and without any interface for custom schedulers, at least that what I take from the experiment). |
I have run performance tests on both commits, what's why I reported the effect in term of peak TPS for each of them, as both help to reduce the number of context switches.
Both changes are needed, that's why I reported the numbers for each, singularly:
There are important gaps on CPU usage, native transport, topology awareness, but as you rightly said, this one is a way to improve what we got by narrowing what the custom scheduler would have achieved. The last meeting your words and Ron ones kept rounding in my head "enable the Virtual Thread where Netty fat poller run to behave like the JDK subpoller" - so I'm trying to not modify the FJP and see at which extent is achievable. That said, let me write down the simplified tables: Using https://github.com/franz1981/Netty-VirtualThread-Scheduler/blob/master/PERFORMANCE.md#io-bound-configuration-max-tps-8-cores with Configs:
Peak TPS (I/O-bound: 8 cores, 30ms mock, 10K connections, NIO, per-request VT)Per-op perf stat countersMassive reduction in context switches (19x vs FJP) and better IPC (+13%). Latency @ 120K req/s (median of 3 runs)Sticky matches FJP on p50, beats it on p99 (85ms vs 98ms), and uses 2% less CPU. |
|
NOTE @AlanBateman these changes work only with |
|
I'm running the same changes in different boxes and configuration to better understand exactly how each config affect the results |
…llers Mode 2 sub-pollers no longer use stickyAffinity — they revert to the original useLazyUnpark dispatch in polled(). Only mode 3 (per-carrier) sub-pollers are sticky.
franz1981
force-pushed
the
sticky_affinity
branch
from
c4f493f to
6e7e35e
Compare
|
@franz1981 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
@AlanBateman @viktorklang-ora I was reading the FJP expected behavior for the sticky case and found semething...weird to me, which I'm still dissecting. with Running this before my changes hangs forever public class StickenssProgress {
@Test
@Timeout(10)
public void testProgress() throws InterruptedException {
testProgress(false);
}
private static void testProgress(boolean sticky) throws InterruptedException {
Assertions.assertTrue(!Thread.currentThread().isVirtual());
// just use booleans to save start/onContinue from sticky threads to be relevant
AtomicBoolean firstStarted = new AtomicBoolean();
AtomicBoolean progressed = new AtomicBoolean();
var factory = Thread.ofVirtual();
if (sticky) {
factory.stickyAffinity();
}
factory.start(() -> {
Thread.yield();
firstStarted.set(true);
while (!progressed.get()) {
Thread.yield();
}
});
while (!firstStarted.get()) {
Thread.onSpinWait();
}
System.out.println("first is started: starting the second one");
Thread secondThread = Thread.ofVirtual().start(() -> {
progressed.set(true);
});
secondThread.join();
}
}I would have expected Thread.yield to submit to the external queue eventually after the second thread, completing both. My first analysis shows that the single worker is trapped in one external queue slot, perpetually draining and refilling the first VT, never scanning the other slots where the second VT has been submitted. FEW NOTES: |
| && ct.getQueuedTaskCount() == 0) { | ||
| externalSubmitRunContinuation(); | ||
| } else { | ||
| submitRunContinuation(); |
There was a problem hiding this comment.
IIUC this can still signal for work despite it would resolve into an internal submission
|
@franz1981 FJP is biased to keep taking work where work is found. In a scenario with a single worker, there's still multiple queues (as you noted). It would be interesting to see how your change behaves under openjdk/jdk#28797 / tagging @DougLea in case he has additional details/thoughts |
|
Thanks @viktorklang-ora
Yep, I see - indeed that's what I'm getting so far; the yield is causing the worker to keep polling against a different "external" worker q from the one where the new VT is submitted, causing the latter to starve. e.g. import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
public class StickinessProgress2 {
public static void main(String[] args) throws InterruptedException {
int carriers = Runtime.getRuntime().availableProcessors();
System.out.println("Carriers: " + carriers);
AtomicInteger started = new AtomicInteger();
AtomicBoolean progressed = new AtomicBoolean();
// Start N yielding VTs to trap all carriers
for (int i = 0; i < carriers; i++) {
Thread.ofVirtual().start(() -> {
Thread.yield();
started.incrementAndGet();
while (!progressed.get()) {
Thread.yield();
}
});
}
while (started.get() < carriers) {
Thread.onSpinWait();
}
System.out.println("All " + carriers + " yielders started, starting rescue VT...");
Thread rescue = Thread.ofVirtual().start(() -> {
progressed.set(true);
});
rescue.join();
System.out.println("Done — progressed");
}
}This seems to hang with 2 and 4 cores but pass with 8, likely because of the number of external queue slots vs carriers. That said, i still see a large amount of unpredictable miss while the worker scan for work with few cores, as it doesn't know which of the 16 slots is available. Probably for FJP we can expect most of the submissions to happen from the delay scheduler/scheduler tp or the master poller - having 16 available slots is quite large for a 2 o 4 cores machine as there's much less contention to spread with so few cores.
I will cherry-pick them and run some tests to see if the benefits I expect are still there 🙏 |
and just to add that this just goes with have no fairness guarantee. |
|
In this case it looks more like a liveness guarantee; as there's someone which cannot ever make any progress (starting too) despite the yield enter back into the runtime |
I have a prototype from some time ago that pushed to a random external queue for the same reason. |
|
I sent Alan a version of lazySubmit with more randomization to check. But to answer other concerns: Unless there is only one core, polling/scanning is almost always better than using a single contended queue, even if some metrics suggest otherwise. |
| * @return this builder | ||
| * @since 99 | ||
| */ | ||
| OfVirtual stickyAffinity(); |
There was a problem hiding this comment.
I'm bad with names...but sticky is because each VT which start or continue through it is locally submitted
|
WDYT about this change @AlanBateman @DougLea ? |
I'll look at it this week. If we expose a method in the prototype for this this then it will require some cleanup and working through some issues with naming and API docs. |
|
Thanks for the updated table. It was initially hard to see how "baseline/sticky", "FJP" and "custom" mapped to the rows "Baseline (no sticky)", "Both commits", "custom no WS" and " FJP reference". If I read the update correctly then changing Thread.yield to use lazySubmit when virtual threads have the "stickyAffinity" bit improves the peak by 15% on this benchmark. The baseline that is the Netty event loops running on virtual threads. I don't mind if we bring this prototype into the fibers branch and probably do some cleanup (do we still need PollerGroup.useLazyUnpark, which was the previous prototype in this area), and think about naming. I think this direction leads to seeing how FJP could handle it if the special bit is in the ForkJoinTask, and maybe see how it could be used to close the gap on MPSC scheduler. |
5 participants
Virtual threads with sticky affinity preserve carrier locality:
externalSubmit, staying on the current carrierPoller.polled()Lazily submitted tasks remain fully stealable (although they lose the wakeup/signaling): stickiness is a locality hint, not a hard pin.
This change to be relevant would need a configuration parameter to allow the built-in scheduler to use the same timer mechanism of the custom scheduler.
All the numbers collected below are NOT using timers on purpose as they change by far the performance results (negatively).
Progress
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/loom.git pull/226/head:pull/226$ git checkout pull/226Update a local copy of the PR:
$ git checkout pull/226$ git pull https://git.openjdk.org/loom.git pull/226/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 226View PR using the GUI difftool:
$ git pr show -t 226Using diff file
Download this PR as a diff file:
https://git.openjdk.org/loom/pull/226.diff
Using Webrev
Link to Webrev Comment