perfomance: promiseState optimization

[PandaWorker]

export async function promiseState(p: Promise<unknown>): Promise<PromiseState> {
	await new Promise<void>(resolve => {
		setTimeout(() => resolve(), 0);
	});
	const t = {};

	return Promise.race([p, t]).then(
		v => (v === t ? 'pending' : 'fulfilled'),
		() => 'rejected'
	);
}

export function promiseState2(
	p: Promise<unknown>
): Promise<PromiseState> {
	return Promise.resolve().then(() =>
		Promise.race([p, null]).then(
			v => (v === null ? 'pending' : 'fulfilled'),
			() => 'rejected'
		)
	);
}

Benchmarks

cpu: Apple M1 Max
runtime: deno 1.45.5 (aarch64-apple-darwin)

file:///Users/panda/Documents/deno/sync/semaphore/Semaphore.ts
benchmark          time (avg)        iter/s             (min … max)       p75       p99      p995
------------------------------------------------------------------- -----------------------------

group perfomance
promiseState        2.32 ms/iter         431.7     (2.26 ms … 2.61 ms) 2.32 ms 2.44 ms 2.6 ms
promiseState2     709.53 ns/iter   1,409,381.1   (673.34 ns … 1.54 µs) 684.64 ns 1.54 µs 1.54 µs

summary
  promiseState2
   3264.49x faster than promiseState

#Tests

running 5 tests from ./Semaphore.ts
promiseState2() returns 'fulfilled' for resolved promise ... ok (0ms)
promiseState2() returns 'rejected' for rejected promise ... ok (0ms)
promiseState2() returns 'pending' for not resolved promise ... ok (0ms)
promiseState2() returns refreshed status ... ok (0ms)

[lambdalisue]

Unfortunately, that doesn't pass the following test

Deno.test(
  "promiseState() returns 'fulfilled' for resolved promise (with null)",
  async () => {
    const p = Promise.resolve(null);
    assertEquals(await promiseState(p), "fulfilled");
  },
);

[PandaWorker]

This version passes all the tests and wins by 3000x performance

export function promiseState2(p: Promise<unknown>): Promise<PromiseState> {
	const t = {};
	return Promise.resolve().then(() =>
		Promise.race([p, t]).then(
			v => (v === t ? 'pending' : 'fulfilled'),
			() => 'rejected'
		)
	);
}

[PandaWorker]

If you use Symbol as a value to check that it gives an upper 5-10% to the previous version

const t = Symbol.for('t');

export function promiseState3(p: Promise<unknown>): Promise<PromiseState> {
	return Promise.resolve().then(() =>
		Promise.race([p, t]).then(
			v => (v === t ? 'pending' : 'fulfilled'),
			() => 'rejected'
		)
	);
}

[PandaWorker]

cpu: Apple M1 Max
runtime: deno 1.45.5 (aarch64-apple-darwin)

benchmark          time (avg)        iter/s             (min … max)       p75       p99      p995
------------------------------------------------------------------- -----------------------------

group perfomance
promiseState        2.31 ms/iter         433.5     (1.16 ms … 3.06 ms) 2.33 ms 2.64 ms 2.73 ms
promiseState2      745.3 ns/iter   1,341,743.6     (701.2 ns … 1.6 µs) 717.07 ns 1.6 µs 1.6 µs
promiseState3     693.04 ns/iter   1,442,921.0 (681.42 ns … 792.55 ns) 690 ns 792.55 ns 792.55 ns

summary
  promiseState3
   1.08x faster than promiseState2
   3328.7x faster than promiseState

[lambdalisue]

Nice. Using Symbol is acceptable (we should not use Symbol.for though)

[PandaWorker]

may be use to have a unique symbol ?

const t = Symbol('t');

export function promiseState3(p: Promise<unknown>): Promise<PromiseState> {
	return Promise.resolve().then(() =>
		Promise.race([p, t]).then(
			v => (v === t ? 'pending' : 'fulfilled'),
			() => 'rejected'
		)
	);
}

[lambdalisue]

Unfortunately, that doesn't pass current test...
#38

[PandaWorker]

That's because my version checks the current state of Promise for a tick trace, and your version only after completing macro/micro tasks

run this code

import { equal as assertEquals } from 'node:assert';

import { Notify } from '@core/asyncutil/notify';
import { promiseState } from '@core/asyncutil/promise-state';

const notify = new Notify();
const waiter1 = notify.notified();
const waiter2 = notify.notified();

assertEquals(notify.waiterCount, 2);

notify.notify();

console.log(1, { waiter1, waiter2 }); // its pending

assertEquals(notify.waiterCount, 1);
assertEquals(await promiseState(waiter1), 'fulfilled');
assertEquals(await promiseState(waiter2), 'pending');

console.log(2, { waiter1, waiter2 }); // its pending

notify.notify();
console.log(3, { waiter1, waiter2 }); // its pending

assertEquals(notify.waiterCount, 0);
assertEquals(await promiseState(waiter1), 'fulfilled');
assertEquals(await promiseState(waiter2), 'fulfilled');

console.log(4, { waiter1, waiter2 }); // its pending

[lambdalisue]

I'm sorry but I don't get your point. The test code you've written seems expected and the new implementation doesn't pass that test.

[PandaWorker]

You probably didn't understand exactly what I meant, look if you don't use promiseState, they are all in the pending status, that is, the current implementation does not show the current Promise status, but waits until all running micro/macro tasks that were launched before calling promiseState are completed, so this implementation is not very productive. If you need a function that would wait for changes that have been started but have not yet been executed. Perhaps it would be good to assign the name promiseStateImmediate to the current implementation, and my implementation, which does not wait, immediately shows the current state of promiseState. I hope I explained it clearly;

import { Notify } from '@core/asyncutil/notify';
import { equal as assertEquals } from 'node:assert';

const notify = new Notify();
const waiter1 = notify.notified();
const waiter2 = notify.notified();

assertEquals(notify.waiterCount, 2);

notify.notify();

console.log(1, { waiter1, waiter2 }); // its pending

// assertEquals(notify.waiterCount, 1);
// assertEquals(await promiseState(waiter1), 'fulfilled');
// assertEquals(await promiseState(waiter2), 'pending');

console.log(2, { waiter1, waiter2 }); // its pending

notify.notify();
console.log(3, { waiter1, waiter2 }); // its pending

//assertEquals(notify.waiterCount, 0);
// assertEquals(await promiseState(waiter1), 'fulfilled');
// assertEquals(await promiseState(waiter2), 'fulfilled');

console.log(4, { waiter1, waiter2 }); // its pending

[lambdalisue]

So, basically, are you saying that it's more about making the behavior more intuitive rather than just improving performance?

[lambdalisue]

Perhaps it would be good to assign the name promiseStateImmediate to the current implementation, and my implementation, which does not wait, immediately shows the current state of promiseState. I hope I explained it clearly;

It sounds the other way around. Since the current implementation includes internal handling that waits for the Promise to resolve, it seems more fitting to describe your implementation, which skips the waiting, as "immediate."

That said, promiseState is primarily designed for use in testing, so I don't plan to change its behavior for performance reasons. However, I do see the value in having an option for cases where waiting isn't desirable, so I'm thinking of adding a feature where setting { immediate: true } would skip the waiting.

P.S.

I updated the PR.

[PandaWorker]

I'm thinking of doing it on Symbol('t') and it will do in principle. We'll, make benchmarks for the implementation of but {} and for Symbol("). But I still think that by default the entire event loop should not wait, and in tests you can also pass a parameter so that it waits.

[lambdalisue]

But I still think that by default the entire event loop should not wait, and in tests you can also pass a parameter so that it waits.

I understand the sentiment, and I would prefer to do that as well, but I feel that it’s not a strong enough reason to introduce a breaking change. If asyncutil were still in the 0.x.y stage, I would have implemented it, but since it’s already at version 1.x.y, I want to avoid making changes that could break backward compatibility as much as possible.

[lambdalisue]

An alternative option is that add peekState that doesn't wait and mark promiseState as an deprecated. I think this is most reasonable.

[PandaWorker]

Okay, then let's better add a separate method currentPromiseState or peekPromiseState, which would not expect an event loop. But then you need to describe both implementations in the documentation and examples and where it is better to use which one

[lambdalisue]

Of course, I’ll document it, but the nuance is a bit different. I plan to treat the current promiseState as deprecated and encourage the use of flushPromises and peekState (or peekPromiseState) in new code.

[PandaWorker]

Yes, I support this decision.