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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Benchmarking async functions? #203

Closed
armanbilge opened this issue Aug 13, 2021 · 9 comments 路 Fixed by #206
Closed

Benchmarking async functions? #203

armanbilge opened this issue Aug 13, 2021 · 9 comments 路 Fixed by #206

Comments

@armanbilge
Copy link
Contributor

Hello again! 馃榿

We're looking for something to benchmark Cats Effect on JS, and of course you have the perfect project for this :) The trickyness for our use-case however is that we need to be able to benchmark an async function, e.g. something that returns a Future or a js.Promise or asynchronously invokes a callback. I took a walk through your source, which is really excellent btw, but it seems like to accommodate this would require quite a bit of changes to the Benchmark, Engine, and Clock interfaces/implementations at least. Is this something you'd be open to, or is there a problem with benchmarking an asynchronous function that I'm not realizing?

Thanks in advance!

@japgolly
Copy link
Owner

Who hello!

I took a walk through your source, which is really excellent btw

Haha really? Thanks! From memory the code here is a little..... "organic" in some of it's design 馃槅

Is this something you'd be open to, or is there a problem with benchmarking an asynchronous function that I'm not realizing?

The only potential problem I can think of is that we're going to lose accuracy in the results because of so many async hops but 1) I can't think of any way to avoid that, 2) all async benchmarks will suffer the same penalty so if you're running a bunch of related async benchmarks, the total time might be a bit less precise but the relative times of each BM as they compare to each other should still be valuable. So yeah totally open to it! In fact I might have a quick play with it now and see if I can just smash it out in under an hour. A lot of the internals are already async from memory, just need to plumb through a bit more async support from the external interfaces.

@armanbilge
Copy link
Contributor Author

Yay! Thanks for being open to this!! 馃榿

all async benchmarks will suffer the same penalty so if you're running a bunch of related async benchmarks, the total time might be a bit less precise but the relative times of each BM as they compare to each other should still be valuable

Yup, that's exactly our situation!

just need to plumb through a bit more async support from the external interfaces.

Yeah, exactly what I thought too!

In fact I might have a quick play with it now and see if I can just smash it out in under an hour.

That would be awesome, thank you! Actually, I had a go at this myself the other day in my fork, and I came out with something usable for our needs. Although be warned, I significantly reworked the insides to use Cats Effect data types I was more familiar with 馃槵 (e.g., your Setup is my Resource Kleisli, and all that awesome cancellation logic in your Engine is directly built into Fiber.

@japgolly
Copy link
Owner

Wow! Dude you're a gun! (Does that slang make sense over there in the US?)

I'm a bit hesitant though about using CE everywhere. Lemme think out loud aobut why...

  • AsyncCallback is super lightweight, CE is much heavier
    • Larger JS output (meh I guess, it's just a BM)
    • CE would be slower I presume but is it really enough to worry about? Part of me feels yes because these are very hot loops
  • SJReact is already a dependency which is why I'm fine to use AsyncCallback, adding CE...
    • Would introduce a new mandatory dependency - the less the better
    • I don't want to be held back by, or run into compatibility problems in the future. I was working on a little side project experiment a little yesterday and was halted by incompatibilities between CE2 and CE3 on libraries I was using. I'd hate to run into those problems in projects like this
  • There's a bit of subtlety that's extremely important for having the UI be responsive while BMs are running. It's not at all obvious and was annoying to get working. We'd need to make sure your changes didn't accidentally clobber that, which would be really easy to do.

@armanbilge
Copy link
Contributor Author

Wow! Dude you're a gun!

Hehe, just a proof of concept 馃槄 Also really needed to benchmark some stuff.

I'm a bit hesitant though about using CE everywhere.

Well, the biggest problem for us is that now we have a circular dependency between the benchmarks in CE and the benchmark lib 馃檭

CE is much heavier

Is it really? You already have a Cats dependency, CE doesn't add too much I thought. To be fair I haven't really explored your AsyncCallback lib. Also BTW CE patches an important bug with the default Scala.js execution context, see scala-js/scala-js#4129

CE would be slower I presume

You may be right about this, but it also prioritizes fairness between multiple interleaved processes.

Plus, Fibers in CE3 are really awesome and I think could solve a lot of problems you hand-coded solutions for :)

But ...

SJReact is already a dependency

Actually I tossed this stuff out 馃槵 sorry! I think it was getting in the way of cross-building for 2.12, and also it wasn't really necessary for the command-line style interface we were looking for. My immediate goals diverged, and of course I did a terrible hack job, I'm sure with more careful planning this wouldn't be necessary.

incompatibilities between CE2 and CE3 on libraries

Yup, sorry about this pain, CE3 is still very new but it's a big step forward on several fronts and definitely going to stick around for a while.

There's a bit of subtlety that's extremely important for having the UI be responsive while BMs are running.

I wonder if that execution context bug I mentioned is related at all to this? Obviously this is important to get right.

So putting aside my hack job (apologies again!!) what I think would be a great way forward would be something like this:

  • maybe split scalajs-benchmark into a couple more modules? A core with just benchmarking stuff that can run in all environments, then a separate module that builds on that to offer the browser interface?
  • I really think CE3 is worth considering as a dependency. At the very least, I wonder if this stuff could be implemented requiring only the Async typeclass from CE3 and then users can use either IO or AsyncCallback? I mean, not critical, since this still creates a circular dependency problem for CE, but a lot of the critical code in your engine could really be simplified I think :)

Anyway thanks for considering this at all, and sharing all your thoughts! I see you've put up a PR, gonna check that out now :)

@japgolly
Copy link
Owner

CE patches an important bug with the default Scala.js execution context
I wonder if that execution context bug I mentioned is related at all to this? Obviously this is important to get right.

I never use execution contexts 馃榾 The issue I had is that there's only one JS thread so when background tasks are chugging away without yielding, the UI becomes unresponsive. Also I want BMs to be as fast as possible so I do sync BMs in little batches before yielding to the UI, it's faster than spanning a task per sync BM instance.

maybe split scalajs-benchmark into a couple more modules? A core with just benchmarking stuff that can run in all environments, then a separate module that builds on that to offer the browser interface?

Yeah we could do that! The code is already separated into core and ui but it all just lives in the same module atm.

I really think CE3 is worth considering as a dependency

For what benefit though? The library already works as is without it. It would be a different discussion if we were starting the library again from scratch but seeing as it all works as is I don't see value in taking on a new major dependency without a good reason. From an external point of view is there some benefit this would bring that I'm not understanding? Oh wait, I'm an idiot, is it that you want to use the library and specify IOs directly? If so I'd prefer to add a tiny typeclass to the core lib, and add an integration module.

Anyway thanks for considering this at all, and sharing all your thoughts!

np! So seeing as the PR provides async BM support I'm gonna merge and close this. If you'd like to continue our discussion about CE integration and more modularisation would you mind raising new issues for them?

@armanbilge
Copy link
Contributor Author

armanbilge commented Aug 16, 2021

I never use execution contexts

Fair enough! But you are certainly using the task queues that back the Execution Contexts. From scala-js/scala-js#4129:

fyi - V8 has separates queues for tasks, microtask queue (used e.g. for promise.then) and macrotask queue (used e.g. for setTimeout):

IIUC the microtask queue is much more performant, but not fair to the macrotask queue. So CE3 uses the macrotask queue for everything via a polyfill which I think accounts for the performance difference.

The issue I had is that there's only one JS thread so when background tasks are chugging away without yielding, the UI becomes unresponsive.

Yup, CE3 solves this problem with auto-yielding :)

The library already works as is without it.

馃憤 Agree, this library is truly great as is!!

np! So seeing as the PR provides async BM support I'm gonna merge and close this.

Please close, you earned it!! 馃榿 Oops you already did :)

@japgolly
Copy link
Owner

you are certainly using the task queues that back the Execution Contexts

hmmm that's a very good point! Man, I'm gonna have to actually pay attention to that aren't I?

Yup, CE3 solves this problem with auto-yielding :)

Really? Have you confirmed that in practice by manually testing in some browsers? If so, that's super, super awesome!

Please close, you earned it!! grin Oops you already did :)

Ah whoops, sorry lol. I merged the PR and Github closed this automatically. I'm working fast! v0.10.0-RC2 already on the way to Maven Central.

@armanbilge
Copy link
Contributor Author

Really? Have you confirmed that in practice by manually testing in some browsers?

Well, I haven't tested it persay with a browser UI. But, there are tests for this in CE3 itself.

The issue I had is that there's only one JS thread so when background tasks are chugging away without yielding

In CE3, every background task is modeled by a Fiber (which btw has cancellation semantics built-in). Each Fiber "runs" an IO and automatically inserts a yield aka IO.cede every N steps, where N is completely configurable (i.e., N = 1 for maximum fairness).

@armanbilge
Copy link
Contributor Author

Btw, to ensure this yielding is fair to tasks queued via e.g. setTimeout is exactly why CE3 polyfills the Execution Context and avoids the default one used by Promise:

It appears that if any outstanding work is available on the promises queue, it will dominate over the timeouts queue.

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 a pull request may close this issue.

2 participants