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

Engine InvokeAsync is implemented for task/valuetask structure #2049

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

baytekink
Copy link

Engine InvokeAsync is implemented for task/valuetask structure

@baytekink
Copy link
Author

What did I do : Remove ManualResetEvent, instead TaskCompletionSource is added. So we give the thread back to thread pool. After that, I added new methods for InvokeAsync, I generally copied the sequantial code and convert it into Task structure. Most of the components/classes uses sequantial methods, but JintAwaitExpression or JintAwaitxxx classes override the async methods. I may call ...Async methods from the sequantial ones (using GetAwaiter().GetResult()), but I did not do that, so we can discuss if we call Async methods by GetAwaiter().GetResult() or do we copy the nearly same code with ...Async ending method.

@SebastianStehle
Copy link
Contributor

I will give you a bonus if this does work and would make async calls easier. But I am afraid it will kill performance. Have you added a benchmark and have you considered ValueTask?

@baytekink
Copy link
Author

baytekink commented Feb 2, 2025 via email

@lahma
Copy link
Collaborator

lahma commented Feb 2, 2025

My main concern here is all the copy-paste, I'm not willing to maintain two copied versions. Also of course, performance should not degrade.

@lofcz
Copy link
Contributor

lofcz commented Feb 2, 2025

A lot of the code currently copy pasted could be generated, I can look into that if the other issues are solved. Scriban also uses async code gen - shouldn't be too hard to implement (I've done similar work recently).

@baytekink
Copy link
Author

My main concern here is all the copy-paste, I'm not willing to maintain two copied versions. Also of course, performance should not degrade.

Returning Task is changed by ValueTask for performance.

Duplicate methods reduced to single: While calling async code from sequantial code, Preserve() method is used for performance (This method is similar to [AsTask()], but it returns the same [ValueTask] instance when this [ValueTask] represents a successful synchronously completed operation ). For ex:

public JsValue Invoke(JsValue value, object? thisObj, object?[] arguments)
{
return InvokeAsync(value, thisObj, arguments).Preserve().GetAwaiter().GetResult();
}

If javascript code does not include any async/await code, this code will continue to work synchronously. The only difference it will create a ValueTask on stack. But it wont be a problem for GC since it was created on stack.

I have not found time to benchmark this code, I will try on this week, but while I try to benchmark, It would be nice if you could give comments on PR. Thx in advance

@baytekink
Copy link
Author

I will give you a bonus if this does work and would make async calls easier. But I am afraid it will kill performance. Have you added a benchmark and have you considered ValueTask?

I now changed Task to ValueTask. Actually i do not think (I wrote think because teorically ValueTask wont be a problem since it is created on stack) it will decrease the performance, but we must see it by benchmarks of course. Thx in advance

@lahma
Copy link
Collaborator

lahma commented Feb 3, 2025

Async state machinery will add a lot of strain for performance. So I wouldn't keep my hopes up with this async API...

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

Successfully merging this pull request may close these issues.

4 participants