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

feat: Add LoadMetrics support for virtual thread executor. #1734

Merged
merged 1 commit into from
Jan 19, 2025

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Jan 19, 2025

Motivation:

LoadMetrics is needed in BalancingDispatcher, which is missing.

Modification:
Add LoadMetrics in the virtual thread executor.

with:

[WARN] [01/20/2025 02:35:27.497] [VirtualThreadPoolDispatcherSpec-pekko.actor.internal-dispatcher-3] [VirtualThreadExecutorConfigurator] 
Failed to get the default scheduler of virtual thread, so the `LoadMetrics` is not available when using it with `BalancingDispatcher`.
Add `--add-opens java.base/java.lang=ALL-UNNAMED` to the JVM options to help this.

@He-Pin He-Pin added this to the 1.2.0 milestone Jan 19, 2025
@He-Pin He-Pin requested review from pjfanning and Roiocam January 19, 2025 04:42
@He-Pin He-Pin force-pushed the loadmetric branch 2 times, most recently from 9731596 to 4c3edf4 Compare January 19, 2025 05:11
@He-Pin He-Pin requested a review from raboof January 19, 2025 14:27
@He-Pin
Copy link
Member Author

He-Pin commented Jan 19, 2025

Should we backport this to 1.1.0 @pjfanning , because the BalancingDispatcher is broken without this?

@pjfanning
Copy link
Contributor

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.

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

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?

@He-Pin
Copy link
Member Author

He-Pin commented Jan 19, 2025

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.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 19, 2025

  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:...

@He-Pin
Copy link
Member Author

He-Pin commented Jan 19, 2025

Event with ModuleLayer, this can not done without any `add-opens.

--add-opens java.base/java.lang=ALL-UNNAMED
--add-opens java.base/java.lang.invoke=ALL-UNNAMED

is at least needed.

@pjfanning If you block this, I must maintain the local fork when I need this feature.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 19, 2025

Do you know if there is a way to access it without --add-opens @fwbrasil , thanks.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 19, 2025

@pjfanning Or we can say, pekko's Virtual threads support for BalancingDispatcher is broken.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 19, 2025

Now with :

[WARN] [01/20/2025 02:35:27.497] [VirtualThreadPoolDispatcherSpec-pekko.actor.internal-dispatcher-3] [VirtualThreadExecutorConfigurator] 
Failed to get the default scheduler of virtual thread, so the `LoadMetrics` is not available when using it with `BalancingDispatcher`.
Add `--add-opens java.base/java.lang=ALL-UNNAMED` to the JVM options to help this.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 19, 2025

@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.

@pjfanning
Copy link
Contributor

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?

@He-Pin
Copy link
Member Author

He-Pin commented Jan 19, 2025

The short answer is no, no in both PRs.

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm

@He-Pin He-Pin merged commit 373c07a into apache:main Jan 19, 2025
9 checks passed
@He-Pin He-Pin deleted the loadmetric branch January 19, 2025 19:52
val field = clazz.getDeclaredField(fieldName)
field.setAccessible(true)
field.get(null).asInstanceOf[ForkJoinPool]
} catch {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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.

2 participants