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

Async function call performance #25

Open
mwojtul opened this issue May 16, 2019 · 4 comments
Open

Async function call performance #25

mwojtul opened this issue May 16, 2019 · 4 comments

Comments

@mwojtul
Copy link
Contributor

mwojtul commented May 16, 2019

I wrote a JS function that fetches some data and uses it to render a React/Vue component. I decided to do some load testing and for consistency's sake I mocked the data call with a promise that returns after 40ms.

50 virtual users sending 5 requests each:

artillery quick --count 50 -n 5 http://localhost:4000/render

Results with a pool size of 4:

Summary report @ 02:09:01(-0500) 2019-05-16
Scenarios launched: 50
Scenarios completed: 50
Requests completed: 250
RPS sent: 84.46
Request latency:
min: 44.3
max: 512.7
median: 357.2
p95: 483.4
p99: 503.2
Scenario counts:
0: 50 (100%)
Codes:
200: 250

I haven't worked much with OTP yet but it seems like each worker needs to wait for the async request to resolve. The same behavior can be observed in test "gets resolved value". Calling NodeJS.call("slow-async-echo", [1234]) a second time will add another second to the test execution time.

@bryanjos Any way around this? I'd be happy to take a stab at a PR or help in whatever way. Figured I'd inquire if anything jumps out to you.

@bryanjos
Copy link
Contributor

@mwojtul nothing really jumps out to me. If by each worker needs to wait for the async request to resolve you mean that it waits for the promise to resolve, I think you are correct. I think that's how it's implemented here today. A PR is welcomed if you decide to give it a shot! I'll assign this to @jwietelmann to keep it on his radar

@jwietelmann
Copy link
Contributor

jwietelmann commented Feb 5, 2020

I know it's been forever since anyone responded, but I have a feeling this is because of our undocumented environment variable optimization: https://github.com/revelrylabs/elixir-nodejs/blob/master/priv/server.js#L6

We blow away require's cache on every call unless you set NODE_ENV=production.

@mwojtul
Copy link
Contributor Author

mwojtul commented Apr 26, 2020

I tried setting NODE_ENV to production, but so far I'm seeing the same results. Maybe I didn't do it correctly? I also edited the server.js file directly as well and didn't observe any improvements, so I'm thinking that might not be it.

@oohnoitz
Copy link
Contributor

oohnoitz commented May 15, 2020

I looked into this some time ago with elixir_react_render and its definitely tied to the way Task works/used.

https://github.com/revelrylabs/elixir-nodejs/blob/master/lib/nodejs/supervisor.ex#L52-L65

As you can see in the code above, NodeJS.call is a pretty much a synchronous blocking call which awaits for the JS code to finish so that we can return the result. If you want to view it as JS, its the equivalent of, which would take about 2s to complete:

async function call() {
  return new Promise.resolve((resolve) =>
    setTimeout(() => resolve(Math.random()), 1000)
  )
}

await call()
await call()

I think we'd need to change or add something to make it not await in the call but return the Task itself. I didn't have that much time to experiment and try out that solution to see if it was valid and solved the problem. This does mean that we'd have to handle the await in another location or towards the end when we render everything after kicking off each task.

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

No branches or pull requests

4 participants