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

refactor: Non-public API for work-stealing from QueueExecutionContext #3863

Merged
merged 14 commits into from Apr 3, 2024

Conversation

WojciechMazur
Copy link
Contributor

@WojciechMazur WojciechMazur commented Apr 2, 2024

Based on discussions in #3853 we propose an alternative, non-public API for working with NativeExecutionContext

The API allows for the WorkStealing capability - this one would be used internally to implement #3860. We expose 2 execution contexts:

  • val queue: ExecutionContextExecutor - standard instance for general usage
  • private[scala.scalanative] val queueInternal: QueueExecutionContext - same instance as the queue but provided extended API

The main purpose here is to provide utilities for internal usages, however, 3rd parties might still defined their integrations via queueInternal if they'll define accessor in the scala.scalanative package of their project. The decision for this approach is to limit interaction with internals and encourage usage of dedicated ExecutionContextExecutors that would not depended on internal Scala Native execution queue.

Only if if the upstream scala.concurrent.ExecutionContext(Executor) would add additional capabilities allowing for scheduling or other interaction with asynchronous execution we would add and expose these in the public API

@catap
Copy link
Contributor

catap commented Apr 3, 2024

The main purpose here is to provide utilities for internal usages, however, 3rd parties might still defined their integrations via queueInternal if they'll define accessor in the scala.scalanative package of their project. The decision for this approach is to limit interaction with internals and encourage usage of dedicated ExecutionContextExecutors that would not depended on internal Scala Native execution queue.

Can you show how does it possible to make similar with scala-native-loop integration via ExecutionContextExecutors? Until I see it I don't agree that it's possible and approach on this PR is simple move to another hack which requires to create a proxy interface inside the scala.scalanative package.

@catap
Copy link
Contributor

catap commented Apr 3, 2024

Only if if the upstream scala.concurrent.ExecutionContext(Executor) would add additional capabilities allowing for scheduling or other interaction with asynchronous execution we would add and expose these in the public API

Scala is designed to be multithreading, and SN has optional multithreading. This is a root cause of all this discussion.

If we have an ability to move event loop into one, single thread which always run, we don't need such API. But when SN is with disabled multithreading, such API is required to implement deep integration.

@WojciechMazur
Copy link
Contributor Author

Can you show how does it possible to make similar with scala-native-loop integration via ExecutionContextExecutors?

Here's the patch that need to be applied to current HEAD of scala-native/scala-native-loop to integrate it with this change.
In this case code was already placed in package scala.scalanative.loop.

diff --git a/build.sbt b/build.sbt
index 8fc66a7..f241072 100644
--- a/build.sbt
+++ b/build.sbt
@@ -1,5 +1,5 @@
 val scala3 = "3.1.1"
-val scala213 = "2.13.4"
+val scala213 = "2.13.13"
 val scala212 = "2.12.13"
 val scala211 = "2.11.12"
 
@@ -39,8 +39,8 @@ lazy val commonSettings = Seq(
     "-Xfatal-warnings",
     // "-Wunused:imports"
   ),
-  libraryDependencies += "com.lihaoyi" %%% "utest" % "0.7.11" % Test,
-  testFrameworks += new TestFramework("utest.runner.Framework"),
+  // libraryDependencies += "com.lihaoyi" %%% "utest" % "0.7.11" % Test,
+  // testFrameworks += new TestFramework("utest.runner.Framework"),
 )
 
 lazy val core = project
diff --git a/core/src/main/scala/scala/scalanative/loop/Eventloop.scala b/core/src/main/scala/scala/scalanative/loop/Eventloop.scala
index cde110b..744b4b6 100644
--- a/core/src/main/scala/scala/scalanative/loop/Eventloop.scala
+++ b/core/src/main/scala/scala/scalanative/loop/Eventloop.scala
@@ -9,32 +9,17 @@ object EventLoop {
 
   val loop: LibUV.Loop = uv_default_loop()
 
-  // Schedule loop execution after main ends
-  scalanative.runtime.ExecutionContext.global.execute(
-    new Runnable {
-      def run(): Unit = EventLoop.run()
-    }
-  )
-
+  
   // Reference to the private queue of scala.scalanative.runtime.ExecutionContext
-  private val queue: mutable.ListBuffer[Runnable] = {
-    val executionContextPtr =
-      fromRawPtr[Byte](castObjectToRawPtr(ExecutionContext))
-    val queuePtr = !((executionContextPtr + 8).asInstanceOf[Ptr[Ptr[Byte]]])
-    castRawPtrToObject(toRawPtr(queuePtr))
-      .asInstanceOf[mutable.ListBuffer[Runnable]]
-  }
+  private val queue = scala.scalanative.concurrent.NativeExecutionContext.queueInternal
+
+  // Schedule loop execution after main ends
+  queue.execute(() =>  EventLoop.run())
 
   def run(): Unit = {
-    while (uv_loop_alive(loop) != 0 || queue.nonEmpty) {
-      while (queue.nonEmpty) {
-        val runnable = queue.remove(0)
-        try {
-          runnable.run()
-        } catch {
-          case t: Throwable =>
-            ExecutionContext.global.reportFailure(t)
-        }
+    while (uv_loop_alive(loop) != 0 || queue.isWorkStealingPossible) {
+      while (queue.isWorkStealingPossible) {
+        queue.stealWork(1)
         uv_run(loop, UV_RUN_NOWAIT)
       }
       uv_run(loop, UV_RUN_ONCE)
diff --git a/project/plugins.sbt b/project/plugins.sbt
index bec6aea..07046e7 100644
--- a/project/plugins.sbt
+++ b/project/plugins.sbt
@@ -1,4 +1,4 @@
-addSbtPlugin("org.scala-native" % "sbt-scala-native" % "0.4.4")
+addSbtPlugin("org.scala-native" % "sbt-scala-native" % "0.5.0-SNAPSHOT")
 addSbtPlugin("com.eed3si9n"     % "sbt-dirty-money"  % "0.2.0")
 addSbtPlugin("com.jsuereth"     % "sbt-pgp"          % "2.0.1")
 addSbtPlugin("org.xerial.sbt"   % "sbt-sonatype"     % "3.9.4")

Until I see it I don't agree that it's possible and approach on this PR is simple move to another hack which requires to create a proxy interface inside the scala.scalanative package.

I wouldn't call it a hack. A hack is the previous approach used in scala-native-loop to get to queue field using an hard coded offset. I would already be broken, in 0.5.0 becouse the object header might be either 8 or 16 bytes. Creating a proxy is trivial (we use them as well between nativelib and javalib to not expose publicly access to internals ), and gives you additional benefit of encapsulating access to the unstable or prone to change parts. After all, when you'll want to yield current execution and iterate on the queue tasks, you/user would not want to touch the internals, but rather use a stable wrapper, which in some cases might be a noop.

We don't have reflection in Scala Native, so this approach of using well known features to limit access to unstable/internal parts seems to be a good middle ground.

The initial refactor of the API in #3144 also had initially access limited to runtime package, although it was to strict at the time. 1d36a10 Yes, it would be great to have an stable API instead of that, but we don't yet have any insight on how Scala 2 stdlib might evolve, and we don't want to make any parts of the API public if we would not be able to confirm we can maintain it.

@catap
Copy link
Contributor

catap commented Apr 3, 2024

In this case code was already placed in package scala.scalanative.loop

So, you encourage users to put their code into package scala.scalanative.XXX am I right? It works well until the first conflict from two different libraries is happened.

@catap
Copy link
Contributor

catap commented Apr 3, 2024

My point: encourage to every user to create dedicated wrapper inside package scala.scalanative is worst than make some code available which is part of scala.scalanative.XXX.

I can agree with use of dedicated ExecutionContext, but nobody still show me the code which doesn't requires hacks like:

  • put some wrapper to get access to internal methods inside SN;
  • magic with rawPtr.

Also, dedicated ExecutionContext make users of scalatest quite unhappy. Do not forget that it uses it's own ExecutionContext to track that tests aren't good.

@WojciechMazur
Copy link
Contributor Author

Our ecosystem is rather small, and the conflicts can be further lowered by using some good practices, eg. if you're code is defined in my.org.event.loop why not define it in package scala.scalanative.interop.my.org.event.loop
Also, the package itself is not yet an issue. The name of the proxy class also would need to match. Yes, there is risk, but how many projects are going to need to integrate with it? We already know that cats-effect will not.

Also, dedicated ExecutionContext make users of scalatest quite unhappy. Do not forget that it uses it's own ExecutionContext to track that tests aren't good.

But, they had no integration with the existing ExecutionContext previously, right? I've tried to check for usages of runtime.loop() in their codebase and found none. No access to raw pointer either. When #3860 is merged calls to Await.result (using LockSupport.park) and Thread.sleep would already iterate the internal queue execution context, and the queue would be also iterated after each delivered messages in the testing interface. These two should be enough to handle the issues found in scalatest. If the tests are correctly structured the SN QueueExecutionContext should probably be never reached anyway. Scalatest defines implicit ExecutionContext in the AsyncTestSuite - in Scala 2 having any other EC in scope would lead to compiler error, not sure about Scala 3

@catap
Copy link
Contributor

catap commented Apr 3, 2024

Await.result (using LockSupport.park) and Thread.sleep would already iterate the internal queue execution context, and the queue would be also iterated after each delivered messages in the testing interface.

This is a direction which I'm investigating right now.

I try to rework my integration to use Thread.yield() or something similar but I can't figure out how to determine when nothing else is left and I should block on the loop.

If you have any idea, I'll be very appricieted.

@catap
Copy link
Contributor

catap commented Apr 3, 2024

@WojciechMazur if you make public just one method NativeExecutionContext.queueNonEmpty which returns value of NativeExecutionContext.queueNonEmpty such integration can be made as nice and cleas as:

    if (NativeExecutionContext.queueNonEmpty) {
      Thread.yield()
      uv_update_time(uvLoop)
      uv_run(uvLoop, uv_run_mode.NOWAIT)
    } else if (isAlive) uv_run(uvLoop, uv_run_mode.ONCE)
    isAlive || NativeExecutionContext.queueNonEmpty

@catap
Copy link
Contributor

catap commented Apr 3, 2024

BTW name queue leads to an expectation that at least isEmpty emthod is available.

@WojciechMazur
Copy link
Contributor Author

Check for nonEmpty makes sense to me, I'll make it available.
When it comes to integration based on Thread.yield() it is in fact based on hack/internal knowledge, because on JVM yield is only a hint to the scheduler, and gives no guarantees about context switching in both system and virtual threads.
It can be used on ScalaNative, but for long term maintenance I'd rather use the access to the internals.

From the JDK 21 docs:

A hint to the scheduler that the current thread is willing to yield its current use of a processor. The scheduler is free to ignore this hint.
Yield is a heuristic attempt to improve relative progression between threads that would otherwise over-utilise a CPU. Its use should be combined with detailed profiling and benchmarking to ensure that it actually has the desired effect.
It is rarely appropriate to use this method. It may be useful for debugging or testing purposes, where it may help to reproduce bugs due to race conditions. It may also be useful when designing concurrency control constructs such as the ones in the java.util.concurrent.locks package.

@catap
Copy link
Contributor

catap commented Apr 3, 2024

@WojciechMazur thanks but this code is required only for non multithreading version, and on that system ignoring Thread.yield() seems quite bad idea, isn't it?

Copy link
Contributor

@catap catap left a comment

Choose a reason for hiding this comment

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

Let"s move on with this because it blocking a lot.

@WojciechMazur WojciechMazur merged commit 4704d58 into scala-native:main Apr 3, 2024
61 checks passed
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.

None yet

2 participants