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.

Sign up for GitHub

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

Optimize allocations #128

Merged
merged 2 commits into from
Feb 26, 2025
Merged

Optimize allocations #128

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
632478a
Avoid allocating `Runnable` wrapper
armanbilge Feb 26, 2025
af120b0
Use ordinary JS object instead of Scala `Map`
armanbilge Feb 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 28 additions & 24 deletions core/src/main/scala/org/scalajs/macrotaskexecutor/MacrotaskExecutor.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@

package org.scalajs.macrotaskexecutor

import scala.collection.mutable
import scala.concurrent.{ExecutionContext, ExecutionContextExecutor}
import scala.scalajs.js
import scala.scalajs.js.annotation._
import scala.util.Random
import scala.util.control.NonFatal

Expand All @@ -29,15 +29,23 @@ object MacrotaskExecutor extends ExecutionContextExecutor {
private[this] final val Undefined = "undefined"

def execute(runnable: Runnable): Unit =
setImmediate(() => runnable.run())
setImmediate(runnable)

def reportFailure(cause: Throwable): Unit =
cause.printStackTrace()

private[this] val setImmediate: (() => Unit) => Unit = {
@js.native
private[this] trait TaskMap extends js.Object {
@JSBracketAccess
def apply(handle: Int): Runnable
@JSBracketAccess
def update(handle: Int, task: Runnable): Unit
}

private[this] val setImmediate: Runnable => Unit = {
if (js.typeOf(js.Dynamic.global.setImmediate) == Undefined) {
var nextHandle = 1
val tasksByHandle = mutable.Map[Int, () => Unit]()
val tasksByHandle = (new js.Object).asInstanceOf[TaskMap]
var currentlyRunningATask = false

def canUsePostMessage(): Boolean = {
Expand All @@ -63,21 +71,17 @@ object MacrotaskExecutor extends ExecutionContextExecutor {
}
}

def runIfPresent(handle: Int): Unit = {
def runTaskForHandle(handle: Int): Unit = {
if (currentlyRunningATask) {
js.Dynamic.global.setTimeout(() => runIfPresent(handle), 0)
js.Dynamic.global.setTimeout(() => runTaskForHandle(handle), 0)
} else {
tasksByHandle.get(handle) match {
case Some(task) =>
currentlyRunningATask = true
try {
task()
} finally {
tasksByHandle -= handle
currentlyRunningATask = false
}

case None =>
val task = tasksByHandle(handle)
Copy link
Member

Choose a reason for hiding this comment

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

With tasksByHandle(handle) now returning a Runnable, there will be an implicit cast to Runnable here that will CCE when it's undefined.

With a custom TaskMap (which really makes sense, btw), you'll get a better mileage by having the apply return a js.UndefOr[Runnable]. Then you could actually just call foreach on it, and you won't need the cast further down. Seems like a win overall.

I wonder how we could better test that each code path actually works. The code paths that are polyfilling really old stuff are under-tested, it seems.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder how we could better test that each code path actually works. The code paths that are polyfilling really old stuff are under-tested, it seems.

Actually, I think the reason we are not hitting the CCE in the undefined case is because it's impossible for us. I don't think we need to consider this case at all.

It was cargo-culted from the upstream, but the upstream also implements clearImmediate.
https://github.com/YuzuJS/setImmediate/blob/f1ccbfdf09cb93aadf77c4aa749ea554503b9234/setImmediate.js#L31-L33

Copy link
Member

Choose a reason for hiding this comment

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

Ah ah! That makes sense. In that case the logic looks good now. We should rename runIfPresent to run, though. Otherwise the next poor soul coming here will be confused.

currentlyRunningATask = true
try {
task.run()
} finally {
js.special.delete(tasksByHandle, handle)
currentlyRunningATask = false
}
}

Expand All @@ -95,7 +99,7 @@ object MacrotaskExecutor extends ExecutionContextExecutor {
js.Dynamic.global.Node.constructor("return setImmediate")()

{ k =>
setImmediate(k)
setImmediate(() => k.run())
()
}
} else if (canUsePostMessage()) {
Expand All @@ -115,7 +119,7 @@ object MacrotaskExecutor extends ExecutionContextExecutor {
.data
.indexOf(messagePrefix)
.asInstanceOf[Int] == 0) {
runIfPresent(event.data.toString.substring(messagePrefix.length).toInt)
runTaskForHandle(event.data.toString.substring(messagePrefix.length).toInt)
}
}

Expand All @@ -129,22 +133,22 @@ object MacrotaskExecutor extends ExecutionContextExecutor {
val handle = nextHandle
nextHandle += 1

tasksByHandle += (handle -> k)
tasksByHandle(handle) = k
js.Dynamic.global.postMessage(messagePrefix + handle, "*")
()
}
} else if (js.typeOf(js.Dynamic.global.MessageChannel) != Undefined) {
val channel = js.Dynamic.newInstance(js.Dynamic.global.MessageChannel)()

channel.port1.onmessage = { (event: js.Dynamic) =>
runIfPresent(event.data.asInstanceOf[Int])
runTaskForHandle(event.data.asInstanceOf[Int])
}

{ k =>
val handle = nextHandle
nextHandle += 1

tasksByHandle += (handle -> k)
tasksByHandle(handle) = k
channel.port2.postMessage(handle)
()
}
Expand All @@ -153,13 +157,13 @@ object MacrotaskExecutor extends ExecutionContextExecutor {
// we're also not going to bother fast-pathing for IE6; just fall through

{ k =>
js.Dynamic.global.setTimeout(k, 0)
js.Dynamic.global.setTimeout(() => k.run(), 0)
()
}
}
} else {
{ k =>
js.Dynamic.global.setImmediate(k)
js.Dynamic.global.setImmediate(() => k.run())
()
}
}
Expand Down