Optimize allocations #128
Optimize allocations #128
Conversation
armanbilge force-pushed the optimize-allocation branch from 7c8e82a to 94dccaa Compare 
| if (js.typeOf(js.Dynamic.global.setImmediate) == Undefined) { | ||
| var nextHandle = 1 | ||
| val tasksByHandle = mutable.Map[Int, () => Unit]() | ||
| val tasksByHandle = new HashMap[Int, Runnable] |
There was a problem hiding this comment.
This is going to reach ju.HashMap for every Scala.js codebase that uses this library, potentially increasing code size. Have you considered using a plain old JS object instead?
There was a problem hiding this comment.
Good point, that's what the upstream does.
https://github.com/YuzuJS/setImmediate/blob/f1ccbfdf09cb93aadf77c4aa749ea554503b9234/setImmediate.js#L9C9-L9C22
armanbilge force-pushed the optimize-allocation branch 6 times, most recently from 5ded57b to 643145f Compare | } | ||
| | ||
| case None => | ||
| val task = tasksByHandle(handle).asInstanceOf[js.UndefOr[Runnable]] |
There was a problem hiding this comment.
I'm pretty sure you need tasksByHandle.selectDynamic(handle) here. As is, you're calling tasksByHandle with handle as an argument.
There was a problem hiding this comment.
Yes, you're right ... messed up the refactor from an initial try with @BracketAccess. But selectDynamic requires a string, so going to try @BracketAccess again
| if (task.isDefined) { | ||
| currentlyRunningATask = true | ||
| try { | ||
| task.asInstanceOf[Runnable].run() |
There was a problem hiding this comment.
If you're going to cast here anyway, using js.UndefOr only to get isDefined is more trouble than it's worth. You can use js.isUndefined(task) instead.
| nextHandle += 1 | ||
| | ||
| tasksByHandle += (handle -> k) | ||
| tasksByHandle(handle) = k.asInstanceOf[js.Any] |
There was a problem hiding this comment.
And here (and below as well) you'll need updateDynamic.
armanbilge force-pushed the optimize-allocation branch from 643145f to 84b4dee Compare | } | ||
| | ||
| case None => | ||
| val task = tasksByHandle(handle) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
armanbilge force-pushed the optimize-allocation branch 2 times, most recently from 5942ce0 to 1abfea2 Compare armanbilge force-pushed the optimize-allocation branch from 1abfea2 to af120b0 Compare 
2 participants
Runnableas() => UnitMapandOption-basedgetwith an ordinaryjs.Object