feat: Add LoadMetrics support for virtual thread executor.#1734
feat: Add LoadMetrics support for virtual thread executor.#1734He-Pin merged 1 commit intoapache:mainfrom
Conversation
9731596 to
4c3edf4
Compare
|
Should we backport this to 1.1.0 @pjfanning , because the BalancingDispatcher is broken without this? |
|
Any idea when this issue was introduced? If it has always been there then patching seems less useful. If we introduced this recently then patching seems more important. |
pjfanning
left a comment
There was a problem hiding this comment.
The add-opens scared me. Is this only needed if you use virtual threads or do we force all Java 17 users to set this?
|
This is the only way to implement it right, the virtual thread is under the surface, and you can not know how many carrier threads are there. |
protected def teamWork(): Unit =
if (attemptTeamWork) {
@tailrec def scheduleOne(i: Iterator[ActorCell] = team.iterator): Unit =
if (messageQueue.hasMessages
&& i.hasNext
&& (executorService.executor match {
case lm: LoadMetrics => !lm.atFullThrottle() // HERE
case _ => true
})
&& !registerForExecution(i.next.mailbox, false, false))
scheduleOne(i)
scheduleOne()
}
}The whole story is:... |
|
Event with ModuleLayer, this can not done without any `add-opens. is at least needed. @pjfanning If you block this, I must maintain the local fork when I need this feature. |
|
Do you know if there is a way to access it without |
|
@pjfanning Or we can say, pekko's Virtual threads support for |
|
Now with : |
|
@pjfanning I think I'm done with it now, just a warning instead. Please don't block it again. If you find anything that can be improved, push it directly. Workday is coming. |
|
To repeat, I don't mind if add-opens are needed for opt-in features like virtual thread support. What I don't want is to force all Pekko users to set them. So the question is does this change force all Pekko users who use Java 17+ to set the new add-opens? |
|
The short answer is no, no in both PRs. |
| val field = clazz.getDeclaredField(fieldName) | ||
| field.setAccessible(true) | ||
| field.get(null).asInstanceOf[ForkJoinPool] | ||
| } catch { |
There was a problem hiding this comment.
In Java 9+, this can be written as
MethodHandles.Lookup lookup = MethodHandles.lookup();
Class clazz = Class.forName("java.lang.VirtualThread");
String fieldName = "DEFAULT_SCHEDULER";
MethodHandles.Lookup vtLookup = MethodHandles.privateLookupIn(clazz, lookup);
VarHandle vh = vtLookup.findStaticVarHandle(clazz, fieldName, ForkJoinPool.class);
return (ForkJoinPool) vh.get();
This still requires the same --add-opens java.base/java.lang=ALL-UNNAMED as the existing code.
So at the moment, it seems that the VarHandle approach doesn't help that much and complicates the code as we need some way to support Java 9 code.
There was a problem hiding this comment.
AT Work, I have a jpmsutil to add dynamic opens at fly, but still it need at least two add opens to work in the first place.
Motivation:
LoadMetricsis needed inBalancingDispatcher, which is missing.Modification:
Add
LoadMetricsin the virtual thread executor.with: