feat: improve async function detection

[sirenkovladd]

remove condition from run check isAsync with fn() instanceof Promise;

[Aslemammad]

I like this change, I see it removing overhead, but did you see that in the results? Have you been able to notice any thing? I'd like to see it, I'm just curious. This PR LGTM, but I'd like to see @mcollina and @Uzlopak's opinion on this.

[Uzlopak]

isAsyncFunction was doing totally perfect sense, as it was fast to determine if we have a function of type async function () {}.

Now isAsyncFunction makes one run of the function and determines various types of async behaviour. Maybe make it two function. Restore the previous isAsyncFunction and create a new isAsyncTask, which does all the magic.

Also, the call instanceof Promise is not enough. Yes it detects built-in Promises. But It should handle PromiseLike. So call && typeof call.then === 'function'

[Uzlopak]

@Aslemammad

Please check discord.

[sirenkovladd]

@Aslemammad

but did you see that in the results

const bench = new Bench({
  iterations: 5e7,
});
bench
  .add('foo3', () => {
    let t = 0;
    for (let i = 0; i < 1e2; i++) {
      t += i;
    }
  })
  .run().then((r) => {
    console.log(r.map((i) => `${i.name}: ${i.result?.totalTime}`).join('\n'));
    console.table(bench.table());
  });

before

❯ node t_main.cjs
foo3: 8199.261972934008
┌─────────┬───────────┬─────────────┬────────────────────┬──────────┬──────────┐
│ (index) │ Task Name │   ops/sec   │ Average Time (ns)  │  Margin  │ Samples  │
├─────────┼───────────┼─────────────┼────────────────────┼──────────┼──────────┤
│    0    │  'foo3'   │ '6,098,109' │ 163.98523945868016 │ '±0.08%' │ 50000000 │
└─────────┴───────────┴─────────────┴────────────────────┴──────────┴──────────┘

after

❯ node t.cjs
foo3: 7497.976737380028
┌─────────┬───────────┬─────────────┬────────────────────┬──────────┬──────────┐
│ (index) │ Task Name │   ops/sec   │ Average Time (ns)  │  Margin  │ Samples  │
├─────────┼───────────┼─────────────┼────────────────────┼──────────┼──────────┤
│    0    │  'foo3'   │ '6,668,465' │ 149.95953474760054 │ '±0.08%' │ 50000000 │
└─────────┴───────────┴─────────────┴────────────────────┴──────────┴──────────┘

[sirenkovladd]

But It should handle PromiseLike So call && typeof call.then === 'function'

I agree with you, I will change it

[Uzlopak]

consider creating a new test file, where you test all aspects of isAsyncTask.

Especially write unit tests, that check if isAsyncTask returns correct values if beforeEach and/or afterEach throw an error.

[sirenkovladd]

@Uzlopak, that's all?

[sirenkovladd]

Do you have any blocks to merge PR?

[Uzlopak]

I would prefer that @kibertoad have a look. I now would just say, that he has time till sunday. If on sunday no feedback is given, we should not block it longer.

[kibertoad]

Appreciate it! will do my best to review asap, busy week so far :-/

[sirenkovladd]

Do you have any other questions about PR?

[kibertoad]

@sirenkovladd Everything else looks good! Can we remove the unnecessary code duplication, though? I still don't see what it adds.

[sirenkovladd]

Sorry, what do you mean by unnecessary code duplication?

[kibertoad]

@sirenkovladd

  private runSync() {
    const taskStart = this.bench.now();
    this.fn();
    const taskTime = this.bench.now() - taskStart;
    return taskTime;
  }

  private async runAsync() {
    const taskStart = this.bench.now();
    await this.fn();
    const taskTime = this.bench.now() - taskStart;
    return taskTime;
  }

This is exactly same code, the only difference is whether we await it or not. This can be handled as a simple condition, without copy-pasting the rest of it.

[sirenkovladd]

OK, i got it but the point is that there are no conditions when calculating the speed of execution (if i add isSync ? call() : await call()) also if I call await when calling synchronous code (to combine these two cases) it will impose extra time on the synchronous function (already tested) which will make the calculation not correct

so I think this is the best way to calculate the execution time correctly

[kibertoad]

OK, I see your point. Not sure that's the tradeoff that I would make, but I see your reasoning, and it's sound.

[Uzlopak]

wait

[Uzlopak]

just for clarification:

we should not have a condition inside the code, where we started the benchmark.

So something you proposed

const taskStart = this.bench.now(); 
if (isAsync) { 
     await this.fn(); 
} else { 
     this.fn(); 
}
const taskTime = this.bench.now() - taskStart; 

would be imho bad. Thats why I am ok with the current solution.

What we actually just need to do is following.

let taskTime = 0
if (isAsync) {
    const taskStart = this.bench.now();
    await this.fn();
    taskTime = this.bench.now() - taskStart; 
} else {
    const taskStart = this.bench.now();
    this.fn();
    taskTime = this.bench.now() - taskStart; 
}

then we dont need the private methods runAsync and runSync, which btw. imho are not a useful abstraction.

[sirenkovladd]

Applied the suggested changes

[Uzlopak]

Will ask @Aslemammad to release ;)