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

Native async implementation of ExecuteAsync and InvokeAsync #731

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

Conversation

Ellested
Copy link

@Ellested Ellested commented May 8, 2020

Unit tests are good, but needs an implementation that shares them with the sync version.
I thought you could maybe just have two separate engines, and have RunTest execute the code on each of them. You could then tell by the stack trace which of them failed, or have an outer assertion that pointed this out. Let me know what you think.

@3da
Copy link

3da commented May 19, 2020

I need this. Can somebody review it and merge?

@3da
Copy link

3da commented May 19, 2020

I am trying to use this in my project. And I have found that it doesn't work if you don't write var before variable.
For example:
var data = load();

So it works correct. And data has correct type.
But if you write
data = load();

Then data has type Task which is incorrect, I guess.

@Ellested
Copy link
Author

Ellested commented May 19, 2020

@3da When you get into this situation, it's because the code branched over to the sync path - so there's still some cases that fall through. If you set a breakpoint on you CLR Task method, then look in the stacktrace from Jint (the CLR stacktrace), it should point out where it branched over to the sync code path. Just look where it starts to not call into the async versions anymore.

I made a commit the other day, that addressed your issue (I think). Try to update and see if it still persist. Otherwise drop the JS code here, and I'll take a look.

@Ellested
Copy link
Author

Regarding the PR it's not ready to be merged - it still needs some work to make the tests shared between the two code paths. I need some input on that part.

But I think in general the maintainers have to decide whether it's to big a burden to maintain two variants of the same code - alternatively drop the sync path and only maintain an async version (which can be used by sync code as well, with some minor performance penalties though).

That's a huge question, so I think I would probably decline it, unless they see a future demand for async - or people starts a riot, because they need it as badly as I do :-)

@3da
Copy link

3da commented May 19, 2020

I included your branch as source code in my project. Because I can't wait while it will be implemented in perfect way.

I will fix small bugs myself.

Can we replace Task with ValueTask for better performance?

@Ellested
Copy link
Author

Ellested commented May 19, 2020

Can we replace Task with ValueTask for better performance?

ValueTask has some shortcomings, and it can go wrong if you fx. query the result twice and similar, so in some circumstances, it might need a little more attention. I would suggest for this to be an optimization that could follow a stable version - where all the fall through cases are spotted and fixed.

Jint is also cross platform/framework, and ValueTask is a .net core feature only AFAIK (which would then require conditional compilation). I think dot.net 5 will solve all of this in a much prettier way :-)

@SebastianStehle
Copy link
Contributor

Nice work @Ellested ... do you know when you have time to finalize this?

@Ellested
Copy link
Author

Ellested commented Jul 6, 2020

Hey Guys, I've been away from development for a while, but now I'm back again.

@russlank - I'll fix the issues you've found. Thanks, and good job :-)

@SebastianStehle - I'm not sure I will ever finish this, as I've realized I cannot use Jint for my project. The project that I'm working on, requires Jint to be re-entrant, which would be way to hard for me to work out alone (I've tried in various ways).

So I've switched to the ClearScript library, though I discarded this to begin with, as it has a lot more moving parts.
But I keep hitting into the wall with Jint, though I love the whole concept around it. It also performs better in general, as it operates directly on the CLR types. ClearScript is a lot slower here, as it requires marshaling.

Regarding this fork, the implementation still needs some work on the unit tests, so they can run the same suite as the existing sync code, so that's one task that needs some attention. But my focus has never been on taking care of this implementation alone. I will need some others to fulfill the mission, and I don't think the Jint maintainers are interested in this - which I fully understand.

So if someone would like to join as maintainers, or take it over, let me know.

@SebastianStehle
Copy link
Contributor

I had a look to your code and I am not sure if I would maintain 2 paths as it is very easy to miss some paths. I think the only option is to make everything async and then you have to go for ValueTask and you get a lot of changes.

@russlank
Copy link

russlank commented Jul 6, 2020

Hi @Ellested & @SebastianStehle,

I can help and contribute to some extent, but I am not very sure about my continuous commitment; it all depends on how much of my spare time I can manage to afford for the contribution, however I would be glad to participate anyway.

Frankly, I have not gone so deeply in reviewing current Jint’s code and understand it fully much myself, so as the beginning I may contribute in start testing, at least in the beginning, however, I am still interested in making it work in async mode, as its current state prevents me from moving forward and using it for all types of my projects, so I believe it worth having async version in place.

Yes, it is sad that the current team members do not include async in their considerations, at least at the moment, as much as they are focusing on performance issues, and this will complicate things a little, because that will require kind of periodic visits to the original developed branch and updating the async path code.

@Ellested
Copy link
Author

Ellested commented Jul 7, 2020

I've updated the implementation with a merge of the upstream dev branch, just to see how difficult it was.

It's not easy, and the main issue is that the sync path is not aware/concerned about the async path. Some of the sync methods are also pretty large, so it needs quite a lot of duplicate code, as the sync functionality is not always separated into fine grained methods. It's not possible to break up the sync code either as this will just create maintenance overhead on that part. So maintenance will be quite hard (though not impossible).

I've spend 3 hours on this, so that's not something I will be continuing to do, as I'm not using Jint at the moment.

I made the sync engine unit test suite, run the async variants as well. It's a little brutal - though better than nothing ;-)

Finally I've changed the virtual async base methods to abstract, to make sure implementations are not overlooked. As always, stack traces are your friend in finding problems, if the execution path switches to the sync code. Hopefully the abstract methods are preventing most of this (or they at least fail fast if not implemented).

@SebastianStehle
Copy link
Contributor

Are there any plans to complete this? Would be an awesome feature. Some kind of easy integration of async calls would be great. Either Promises or Async execution. I think I can also sponsor this feature.

@Ellested
Copy link
Author

Ellested commented Feb 6, 2021

Are there any plans to complete this? Would be an awesome feature. Some kind of easy integration of async calls would be great. Either Promises or Async execution. I think I can also sponsor this feature.

Hi Sebastian,

I don't think Sebastian Ros and Marko Lahma are interested in the current implementation, as it requires two almost identical code paths to be maintained.

I'm using the code as it is right now, and haven't synchronized it for a while. Mostly because it's seems to work quite stable at this point, and secondly because I'm working on something else :-)

I was thinking about implementing Promises on top of this, but I saw another implementation which is already pending as a PR.
I couldn't personally use that implementation, as I'm using the engine in a single threaded synchronization context (which basically translates to "you cannot await another thread"). The code in the PR therefore also use locking mechanics to avoid re-entrance, and this would be fine in most circumstances. Locking could however be avoided using a pure async implementation and a single threaded synchronization context.

So for now I decided not to do anything, and leave the await keyword unimplemented. Instead I use a wrapper method await(() => ), which saves the execution context, and restores it on return from the async method.

Now that .net 5.0 is here, I think it would make sense to upgrade to ValueTasks, but it would disable the support for classic .net framework.

So many "big" questions and decisions quickly arises, like framework support and so on. A full implementation would have a rather large impact on the main branch, and probably to much concern at the moment.

The best I can do is probably to keep this branch in sync with main, and the maintainers could maybe take a look someday and use some of it for inspiration (when they are ready to make the move).

I see two ways to overcome the duplicate code paths problem. The easiest is just to skip the sync path and only keep the async version. Alternatively you could restructure/refactor the existing code, to minimize the code overlap between the two paths, mainly by refactoring methods into smaller parts (but inline them). But it should be done so it makes logical sense and keeps the readability.

Here's my await() wrapper method (exposed in JS):

private async Task<JsValue> Await(JsValue call)
{
    // Store and leave the current execution context while performing an async operation
    var saveLexEnv = _engine.ExecutionContext.LexicalEnvironment;
    var saveVarEnv = _engine.ExecutionContext.VariableEnvironment;
    _engine.LeaveExecutionContext();
    try
    {
        if (call is ArrowFunctionInstance afi) return await afi.CallAsync(call, new JsValue[] { });
        throw new ScriptEngineException("Error: only arrow functions (closures) are allowed within await()");
    }
    finally
    {
        // Return to the original context when continuing
        _engine.EnterExecutionContext(saveLexEnv, saveVarEnv);
    }
}

@SebastianStehle
Copy link
Contributor

I see it like you. A consistent refactoring to use ValueTask would solve the problem. Then promises are not needed anymore for most uses, you can just have async calls in the script that look like synchronous calls.

@Ellested
Copy link
Author

Ellested commented Feb 7, 2021

I see it like you. A consistent refactoring to use ValueTask would solve the problem. Then promises are not needed anymore for most uses, you can just have async calls in the script that look like synchronous calls.

Yes - though I still think Promises should be implemented on top, for compatibility with both tools and code. I also think it would be easier to do if the engine was async. I think it only requires handling of succes/failure branching on top of the async call mechanics.

I might try to do an implementation later, I think I can look at the PR I mentioned for a lot of inspiration. I like that PR in many ways, but an async implementation would probably be more straight forward as there would be no thread management.

@Ellested
Copy link
Author

I will synchronize this branch within a month or so. But I just updated the method wrapper that I use from the JavaScript code to allow ICallable's instead.

        private async Task<JsValue> Await(JsValue call)
        {
            // Store and leave the current execution context while performing an async operation
            var saveLexEnv = _engine.ExecutionContext.LexicalEnvironment;
            var saveVarEnv = _engine.ExecutionContext.VariableEnvironment;
            _engine.LeaveExecutionContext();
            try
            {
                var thisObj = saveVarEnv.Record.GetThisBinding();
                if (call is ICallable ca) return await ca.CallAsync(thisObj, new JsValue[] { });
                throw new JavaScriptException("Error: only callable functions and methods are allowed as arguments to await()");
                //if (call is ArrowFunctionInstance afi) return await afi.CallAsync(call, new JsValue[] { });
                //throw new JavaScriptException("Error: only arrow functions (closures) are allowed within await()");
            }
            finally
            {
                // Return to the original context when continuing
                _engine.EnterExecutionContext(saveLexEnv, saveVarEnv);
            }
        }

Add the method to the engine:
_engine.SetValue("await", new Func<JsValue, Task>(Await));

From JS you need to use it when you call other JS methods from within you async method.

function method1Async() {
    // like
    await(method2Async);
    // or
    await(() => method2Async());
    
    // for async CLR methods just call them like
    Task.Delay(1000);
    // or
    await(() => Task.Delay(1000));
    // this will fail
    await(Task.Delay(1000));
}

function method2Async() {
    // do stuff
}

I'm a little in doubt whether I can always access the .GetThisBinding() like this, but currently my async JS code works.

@Ellested
Copy link
Author

Ellested commented Mar 1, 2021

Updated the branch with latest dev, and moved ALL async variants into separate files, which makes it a lot easier to manage in the future. Just required fiddling with the Build props file, to enable the VS2019 feature to dynamically fold files. Great feature by the way to organize larger classes, even small things like members are sometimes nice to just have in a separate file, when you reach a certain number. It's the perfect feature for the async code path, the code is clear and isolated now 😀

I have more to come, and already found an issue in this implementation that should be fixed. Namely the stack reset call in the ExecuteAsync method, which prohibits reentrancy. It's fine to remove it, I've tested over a long period. But I need to update my own project first, to make it compatible, as exercising Jint from this project is a really good test. I also spotted some new interesting Engine methods that I need to hook into and test along the way. I will include my JavaScript await wrapper as well, and I think I will probably also try to see if I can implement the native JavaScript async/await into this branch. I will need it anyway, as I can't use a Task.Run() based implementation, which seems the normal goto choice. (I only have a single thread available).

The reason that I haven't fixed the above immediately, is that I have another branch that I'm actually using for my own project, that corrected some of this. But it has some overrides and hacks around that wouldn't work for others. Some of the Jint classes that I need to customize, are internal and sealed which hinders direct subclassing and customization from outside the source code. So it makes me run with a mix of code that is not suited for everyone on a separate branch, and I therefore don't go back this branch with my latest findings.

But I think I can improve on that by just subclassing my points of interest with public wrappers in the jint source, and have the bulk part of the code modifications reside in my own projects. This will keep a minimal footprint in the Jint source, so I can keep this branch more up to date. It will depend on how much member promotion I need to do.

Talking about up to date, it's fantastic to see all the great features they've added since my last update - this project rocks 😀
I will go straight in now and make some JS classes 🥇

PS. ValueTask is in consideration, but then it's only .net 5 & .net core. afaik

@lofcz
Copy link

lofcz commented Sep 5, 2021

@Ellested Any news on this? I've tried your branch and it works fine so far. Just curious whether you've progressed further :)

@Ellested
Copy link
Author

Ellested commented Sep 5, 2021 via email

@lofcz
Copy link

lofcz commented Feb 18, 2024

@Ellested I wonder whether you moved to another adventures after so much time has passed. There was 3.0 release of Jint a while back and many parts were shifted around a bit. Is the current code in your Async branch the final version of your work? There are a few tests that don't pass. I'm running Blazor based setup too and while Jint now has rudimentary Task-Promise conversion there are still issues with it, so I'd like to bring your branch up to date.

@Ellested
Copy link
Author

Ellested commented Feb 18, 2024 via email

@lofcz
Copy link

lofcz commented Feb 18, 2024

Thanks for a fast and frankly, better than expected reply! While 3.00-2046 is about a year old build, it would be a much better starting point for sure. I agree with the current upstream implementation being suboptimal and I also like how in this PR the need to await calls is not forced on end-users, making the language more accessible. We designed it the same way in WattleScript.

As for my use case, I'm currently working on an "AI engine" app in which Human-LLM interactions are scripted in JS, here is a quick preview: https://www.youtube.com/watch?v=d20lTP-ToiU
The app aims to abstract all UI away from end users and enable fast creation of scripts that do things from managing the self-supervised work of students to acting as a natural language terminal.
The code editor used is called Monaco and I've implemented a good chunk of DAP for it using Jither's work as a starting point.

The reason this is JS-based and not TS is to enable debugging as currently, Esprima (probably to be replaced by Acornima) doesn't currently support sourcemaps. The app itself is server-sided Blazor, I have a protocol developed for it allowing for the efficient two-way binding of long strings (such as code files) between client and server, based on DMP which I could contribute in case Jzor targets Server and doesn't have this solved already.

If you would like my help with anything from updating Jint to creating more tests, let me know and I'll contribute!

PS: back in 2021 when I inquired about the async support I worked on a very similar use case to this:

It's actually called Jzor, and will be out in an alpha release within a month or so. My project has a built-in TypeScript compiler, which transpiles to JS, which ends up in Jint, and it uses a React like framework and approach to build server side web applications with direct integration to the CLR world.

Ended up designing and using WattleScript back then due to various reasons, but for this project using a standard language and a way better IDE-like tooling available with it overweights the benefits WS brings in. The amount of effort constantly poured into Jint solved quite a few problems that were show-stoppers for me since then.

Instead of React my solution used plain Scriban templates (no reactivity) and was quite clunky to use due to very limited debugging capabilities.

@Ellested
Copy link
Author

Ellested commented Feb 18, 2024

Thanks for a fast and frankly, better than expected reply! While 3.00-2046 is about a year old build, it would be a much better starting point for sure. I agree with the current upstream implementation being suboptimal and I also like how in this PR the need to await calls is not forced on end-users, making the language more accessible. We designed it the same way in WattleScript.

The current version I have, do actually conform and require you to use the await/async patterns. However, it might be possible to add an option that could await "automagically", like the old implementation had.

As for my use case, I'm currently working on an "AI engine" app in which Human-LLM interactions are scripted in JS, here is a quick preview: https://www.youtube.com/watch?v=d20lTP-ToiU The app aims to abstract all UI away from end users and enable fast creation of scripts that do things from managing the self-supervised work of students to acting as a natural language terminal. The code editor used is called Monaco and I've implemented a good chunk of DAP for it using Jither's work as a starting point.

Interesting - I've also implemented most of the DAP, and Jzor can debug multiple applications running on the host simultaneously. I haven't been working on the Jzor debugger for a while, and there's some small issues I need to address when using the debugger command line, but for now it's pretty stable and will do fine for the alpha version.

I also started out with Jithers work, and combined it with the DAP protocol from another library, and ended up with a somewhat tailored version again, as each Jzor application runs on it's own debugger thread in VSCode. This needed a bit of customization to get working, and again I'm not sure it will work for everyone. But I plan also to released this back to github, as I think it could be a good starting point to create a fully working implementation for Jint. I have covered most of the basic DAP stuff, so it's the more exotic features that are missing now, but I think it shouldn't take too long to finish.

The reason this is JS-based and not TS is to enable debugging as currently, Esprima (probably to be replaced by Acornima) doesn't currently support sourcemaps. The app itself is server-sided Blazor, I have a protocol developed for it allowing for the efficient two-way binding of long strings (such as code files) between client and server, based on DMP which I could contribute in case Jzor targets Server and doesn't have this solved already.

If you would like my help with anything from updating Jint to creating more tests, let me know and I'll contribute!

PS: back in 2021 when I inquired about the async support I worked on a very similar use case to this:

It's actually called Jzor, and will be out in an alpha release within a month or so. My project has a built-in TypeScript compiler, which transpiles to JS, which ends up in Jint, and it uses a React like framework and approach to build server side web applications with direct integration to the CLR world.

Ended up designing and using WattleScript back then due to various reasons, but for this project using a standard language and a way better IDE-like tooling available with it overweights the benefits WS brings in. The amount of effort constantly poured into Jint solved quite a few problems that were show-stoppers for me since then.

Instead of React my solution used plain Scriban templates (no reactivity) and was quite clunky to use due to very limited debugging capabilities.

Sounds a lot like Jzor's destiny, It also started as JavaScript only, but I started over and made it TypeScript+React instead :-)
I'm working on the Jzor documentation now, and you can take a look when I'm done (in abouth a month or so) to see if it there's anything you like. At least it would be nice with some feedback :-)

I'll find the branch with my current async Jint implementation this week, and put it up. I then suggest you take a look at it, and we can see how we could proceed, and figure what can be done to make it open to both Jzor and other uses.

@lofcz
Copy link

lofcz commented Feb 18, 2024

Looking forward to your async branch, as currently, the issues with upstream async methods implementation force me to use out-of-process V8 in lieu of Jint. Could you please let me know when it's up? Thanks a lot :-)

@tom-b-iodigital
Copy link
Contributor

@lofcz I extensively use async await with jint and just made a PR to make it a bit more easy to expose async Task returning member methods to jint. Which part of the async/await implementation is not working for you?

@lofcz
Copy link

lofcz commented Feb 21, 2024

@tom-b-iodigital thanks for your PR! I've tested it and the main issue with async implementation I had - CLR tasks not being awaited correctly seems to be fixed. I'm still very interested in the possibility of freeing the executing thread while the task is running (native ExecuteAsync) but at least I can use Jint now.

@tom-b-iodigital
Copy link
Contributor

@lofcz my PR honestly didn't change that much, the entire task to promise and UnwrapIfPromise structure was already in place. I was just tired of always wrapping my objects with static functions and using the engine setvalue method to pass them. My PR made that part easier, now I have a fetch interface that is 99% compatible with the Javascript browser implementations and is completely async.

I think most of the confusion stems from the fact that the docs are not that clear on how to use async/await/tasks/promises together. So I think I will add that part to the Readme with some examples

@Ellested
Copy link
Author

@lofcz I extensively use async await with jint and just made a PR to make it a bit more easy to expose async Task returning member methods to jint. Which part of the async/await implementation is not working for you?

I use Jint with the Blazor framework, which is single threaded in nature. So I cannot use features like Task.Wait and Task.Run at all (which I believe the current implementation still does), as this will lead to a thread lock.

Instead I switch the engine context when awaiting a CLR task inside the engine, which hands the thread back to C#, which is then free to call into the engine again (re-entrance, and still using the same thread). This also mean the implementation is completely lock free,

For large scripts (like executing the TypeScript compiler) the async path inside the engine is noticably slower, but I use it for small scripts like event handlers. I have not done any optimizations, but I think it could be optimized a great deal. Especially if the GetValue had two variants (a fast and slow), as I recall making this async, impacted almost all code inside the engine.

I don't know if this approach has any interest outside my scope, but I'll dump a copy when I have had time to clean it up :-)

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.

None yet

6 participants