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

[Bug] Using async functions degrades performance #1587

Open
c0sx opened this issue Dec 13, 2024 · 4 comments
Open

[Bug] Using async functions degrades performance #1587

c0sx opened this issue Dec 13, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@c0sx
Copy link

c0sx commented Dec 13, 2024

What are you really trying to do?

Hi, I have a workflow like dsl-interpreter from sample and I found that my workflow is slow.

Describe the bug

For some reason using async functions in workflows is drastically slower than sync functions. Could you help me?

I have 2 identical workflows (synthetic examples), the difference only in using async in functions:

// sync workflow
async function run() {
  const iters = () => 0;
  const condition = (iters) => iters < 10_000;
  const incr = (iters) => iters + 1;
  const out = (iters) => iters;

  let i = iters();

  while (condition(i)) {
    i = incr(i);
  }

  return out(i);
}

// async workflow
async function run() {
  const iters = async () => 0;
  const condition = (iters) => iters < 10_000;
  const incr = async (iters) => iters + 1;
  const out = async (iters) => iters;

  let i = await iters();

  while (condition(i)) {
    i = await incr(i);
  }

  return await out(i);
}

My worker and client

// worker.js
const path = require('path');

const { Worker } = require('@temporalio/worker');

async function main() {
  const worker = await Worker.create({
    workflowsPath: path.join(__dirname, './workflows.js'),
    taskQueue: 'hello-javascript',
  });

  await worker.run();
}

main().catch((err) => {
  console.error(err);
  process.exit(1);
});

// client.js
const { Client, Connection } = require('@temporalio/client');

const { run } = require('./workflows');

async function main() {
  const connection = await Connection.connect({});

  const client = new Client({
    connection,
  });

  console.time('client'); // start timer

  const result = await client.workflow.execute(run, {
    taskQueue: 'hello-javascript',
    workflowId: 'my-business-id',
    args: ['Temporal'],
  });

  console.timeEnd('client'); // end timer
  console.log(result);
}

main().catch((err) => {
  console.error(err);
  process.exit(1);
});

And I have this results

Sync workflow Async workflow
25-30ms 350-400ms

Environment/Versions

  • OS and processor: M3 Mac
  • Temporal Version:
temporal -v
temporal version 1.0.0 (Server 1.24.2, UI 2.28.0)
  • SDK: typescript-sdk: 1.10.3

  • Are you using Docker or Kubernetes or building Temporal from source?
    I installed temporal from brew

@c0sx c0sx added the bug Something isn't working label Dec 13, 2024
@c0sx
Copy link
Author

c0sx commented Dec 16, 2024

I found that this problem related with promiseHooks in vm-shared worker here.

Is it really important to handle any promise?

@mjameswh
Copy link
Contributor

I found that this problem related with promiseHooks in vm-shared worker here.

Yeah, I had the intuition the issue you described would be related to this promiseHook.

If you want to dig a bit more, I would expect that the largest part of that extra time comes from these lines, which gets called from the promise hook (see the .stack, which internally calls the prepareStackTrace function).

Is it really important to handle any promise?

The goal of that hook is to capture the data we need for the __stack_trace and __enhanced_stack_trace Workflow queries, which are called from the Temporal UI to help diagnose Workflow issues. That's a nice to have feature, but definitely not required. In fact, promiseHooks is not supported on Node <16.14, in which case that hook simply doesn't get registered.

Adding a worker config option to turn off this feature would be a possibility.

I would however like to investigate first what can be done to reduce the runtime overhead of capturing stack traces on async contexts. I have a few ideas in mind that, I believe, could reduce the cost to nearly zero, without compromising on availability of this feature.

@c0sx
Copy link
Author

c0sx commented Dec 24, 2024

@mjameswh Hi, do you have any news?

@mjameswh
Copy link
Contributor

No, not at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants