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

Improve performance for async functions #29

Open
JulienLecoq opened this issue Feb 18, 2022 · 8 comments
Open

Improve performance for async functions #29

JulienLecoq opened this issue Feb 18, 2022 · 8 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@JulienLecoq
Copy link

JulienLecoq commented Feb 18, 2022

In some async functions, you're using redundant code that makes the execution code slower (I assume that this is also valid for the deno sdk). I didn't new at first that this redundant code could make the execution slower, but I tested it and here is my experience/results.

In some async functions, you're using the following pattern (from /lib/services/account.js):

async get() {
  let path = '/account';
  let payload = {};

  return await this.client.call('get', path, {
      'content-type': 'application/json',
  }, payload);
}

This pattern contains an unnecessary "async" and "await" notation.
This could be simplified by:

get() {
  let path = '/account';
  let payload = {};

  return this.client.call('get', path, {
      'content-type': 'application/json',
  }, payload);
}

From this pov, I decided to benchmark the 2 solutions with the following script:

async function measure_async_func(cb) {
    const start = performance.now()

    await cb()

    const stop = performance.now()
    const timeInSeconds = (stop - start) / 1000;
    const timeRounded = timeInSeconds.toFixed(3)

    return timeRounded
}

function printExecutionTime(funcName, executionTime) {
    console.log(`Time taken to execute function ${funcName}(): ${executionTime} seconds`)
}

function getNumber() {
    return new Promise((resolve, _) => {
        resolve(10)
    })
}

function getNumber1() {
    return getNumber()
}

async function getNumber2() {
    return await getNumber()
}

async function getNumbers1(numbersToGenerate) {
    for (let i = 0; i < numbersToGenerate; i++) {
        await getNumber1()
    }

}

async function getNumbers2(numbersToGenerate) {
    for (let i = 0; i < numbersToGenerate; i++) {
        await getNumber2()
    }
}

async function runTests() {
    const numbersToGenerate = 10_000_000

    const executionTime1 = await measure_async_func(() => getNumbers1(numbersToGenerate))
    printExecutionTime("getNumbers1", executionTime1)

    const executionTime2 = await measure_async_func(() => getNumbers2(numbersToGenerate))
    printExecutionTime("getNumbers2", executionTime2)

    const ratio = executionTime2 / executionTime1
    console.log(`getNumbers1() is ${ratio} faster than getNumbers2()`)
}

runTests()

The results are (with Node v17.4.0):

Time taken to execute function getNumbers1(): 0.928 seconds
Time taken to execute function getNumbers2(): 1.613 seconds
getNumbers1() is 1.738146551724138 faster than getNumbers2()

I haven't tested with the sdk itself, but with a simple script, the second solution (without "async" and "await" notation) seems 1.74 times faster than the first one. I should have tested with the sdk itself, but I don't really have time for that rn.

@JulienLecoq JulienLecoq changed the title Improve performance with async functions Improve performance for async functions Feb 18, 2022
@jatinmark
Copy link

i want to work on this issue , can you assign it to me.

@ChiragAgg5k ChiragAgg5k added good first issue Good for newcomers help wanted Extra attention is needed labels Jan 29, 2025
@ChiragAgg5k
Copy link
Member

This is a good issue and open for contributions, make sure to checkout our sdk generator repo and raise a PR there - https://github.com/appwrite/sdk-generator

@sahilsekr42
Copy link

Hello @ChiragAgg5k , might be really helpful if you could confirm the need of resolving this issue.

@ChiragAgg5k
Copy link
Member

@sahilsekr42 the issue is still open for contribution! feel free to open a pr on the sdk generator repository. Make sure to read the guidelines before making the PR. Would also greatly appreciate if you can link the benchmark results before and after making the changes.

@sahilsekr42
Copy link

Ok , on it. and thanks for quick reply .

@sahilsekr42
Copy link

Hey @ChiragAgg5k, I performed this little experiment of compiling and building 2 versions of the node-sdk. Then packaging and importing both versions into Node projects running the same backend code ( which was basically adding documents to an existing appwrite database collection ). The only change that I did in both the versions was removed the redundant async await function from createDocument. Then I performed the tests for different number of documents similar to what was recommended by @JulienLecoq .

Here are results (in ms) for comparision :-- 1749.131 vs 1532.828 ; 119.339 vs 109.946 ; 160.056 vs 147.534 ; 171.254 vs 152.308
which gives an average execution boost of roughly 11% .
Now, although this is not as huge as expected but I think with functions not dealing with database and just users , accounts etc. this might be more.

@sahilsekr42
Copy link

Performing similar steps for benchmarking create user functions for auth I observed improvement of around 9% just by removing a couple of async/await. Should I first raise an issue then PR in sdk-generator repo now ? @ChiragAgg5k

@ChiragAgg5k
Copy link
Member

@sahilsekr42 feel free to create the PR directly and link this issue in the PR. Thanks for the benchmark numbers, they look really promising 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants