sebastienros / esprima-dotnet

Esprima .NET (BSD license) is a .NET port of the esprima.org project. It is a standard-compliant ECMAScript parser (also popularly known as JavaScript).
BSD 3-Clause "New" or "Revised" License
425 stars 75 forks source link

Problem with the latest build on Xamarin/iOS #344

Closed munichmule closed 1 year ago

munichmule commented 2 years ago

Hi Guys,

First of all I want to thank you for amazing projects you develop, Jint and Esprima worked well for me for many years.

I upgraded some older Xamarin app to the Jint@3.0.0-beta-41 and Esprima@3.0.0-beta-7 a few days ago, and it started to fail in runtime on iOS. If I downgrade to Jint@3.0.0-beta-39 and Esprima@3.0.0-beta-3, the problem goes away.

I didn't dig super-deep into it, but it looks like this commit using open delegates for parser methods caching might be the root cause.

I understand Xamarin/iOS is not 1st class citizen, but the library was fully compatible with it, and I really enjoyed that because it allowed me to use the same script execution engine across different platforms and runtime versions. Do you think it might be fixed in the library?

Thanks in advance!

System.ExecutionEngineException: Attempting to JIT compile method '(wrapper delegate-invoke) Esprima.Ast.Expression <Module>:invoke_callvirt_Expression_JavaScriptParser (Esprima.JavaScriptParser)' while running in aot-only mode. See https://docs.microsoft.com/xamarin/ios/internals/limitations for more information.

at Esprima.JavaScriptParser.ParseLeftHandSideExpressionAllowCall () <0x1061aa080 + 0x00694> in <0b544c2cdd8e4af3a57b8d45fca589f1#bef47091073c88a40e760639b4f843c2>:0 
at Esprima.JavaScriptParser.ParseUpdateExpression () <0x1061ac7e0 + 0x00287> in <0b544c2cdd8e4af3a57b8d45fca589f1#bef47091073c88a40e760639b4f843c2>:0 
at Esprima.JavaScriptParser.ParseUnaryExpression () <0x1061ad3b0 + 0x001ef> in <0b544c2cdd8e4af3a57b8d45fca589f1#bef47091073c88a40e760639b4f843c2>:0 
at Esprima.JavaScriptParser.ParseBinaryExpressionOperand () <0x1061adb00 + 0x001a3> in <0b544c2cdd8e4af3a57b8d45fca589f1#bef47091073c88a40e760639b4f843c2>:0 
at Esprima.JavaScriptParser.ParseBinaryExpression () <0x1061ae6e0 + 0x00173> in <0b544c2cdd8e4af3a57b8d45fca589f1#bef47091073c88a40e760639b4f843c2>:0 
at Esprima.JavaScriptParser.ParseAssignmentExpression () <0x1061b0940 + 0x0028b> in <0b544c2cdd8e4af3a57b8d45fca589f1#bef47091073c88a40e760639b4f843c2>:0 
at Esprima.JavaScriptParser.IsolateCoverGrammar[T] (System.Func`2[T,TResult] parseFunction) <0x10619fd60 + 0x0006b> in <0b544c2cdd8e4af3a57b8d45fca589f1#bef47091073c88a40e760639b4f843c2>:0 
at Esprima.JavaScriptParser.ParseVariableDeclaration (System.Boolean inFor) <0x1061b5da0 + 0x00363> in <0b544c2cdd8e4af3a57b8d45fca589f1#bef47091073c88a40e760639b4f843c2>:0 
at Esprima.JavaScriptParser.ParseVariableDeclarationList (System.Boolean inFor) <0x1061b6280 + 0x0004f> in <0b544c2cdd8e4af3a57b8d45fca589f1#bef47091073c88a40e760639b4f843c2>:0 
at Esprima.JavaScriptParser.ParseVariableStatement () <0x1061b64c0 + 0x00147> in <0b544c2cdd8e4af3a57b8d45fca589f1#bef47091073c88a40e760639b4f843c2>:0 
at Esprima.JavaScriptParser.ParseStatement () <0x1061bdde0 + 0x0062f> in <0b544c2cdd8e4af3a57b8d45fca589f1#bef47091073c88a40e760639b4f843c2>:0 
at Esprima.JavaScriptParser.ParseStatementListItem () <0x1061b1e50 + 0x002ab> in <0b544c2cdd8e4af3a57b8d45fca589f1#bef47091073c88a40e760639b4f843c2>:0 
at Esprima.JavaScriptParser.ParseScript (System.String code, System.String source, System.Boolean strict) <0x10619d830 + 0x000a3> in <0b544c2cdd8e4af3a57b8d45fca589f1#bef47091073c88a40e760639b4f843c2>:0 
lahma commented 2 years ago

Thanks for the report. Being AOT friend on NET 7 would be nice too (probably unrelated to this matter. What do you think @adams85 , should we revert the delegate change?

adams85 commented 2 years ago

Technically, we could workaround Delegate.CreateDelegate (which is causing the problem) with C# 9 function pointers but one of the cached methods (ParsePrimaryExpression) is virtual, which we'd need to wrap in a static method, which would increase call stack depth and probably cause a minor regression in performance. It's a bummer because function pointers could otherwise bring 1-2% improvement in execution time...

So yeah, I don't have any better idea than reverting.

adams85 commented 2 years ago

I couldn't resist some experimentation with function pointers and it seems they allow us to kill two birds with one stone (see #345).

In principle, this should solve this AOT-only issue, however I don't have an iOS device at hand. So, @h15ter, could you test the proposed solution? Just to make sure.

adams85 commented 1 year ago

So, @h15ter, could you test the proposed solution? Just to make sure.

345 turned out to be a dead-end, don't bother with it. The original approach was restored by #346.

munichmule commented 1 year ago

Amazing! Thank you very much.

munichmule commented 1 year ago

@lahma Do you think there will be a next release of Esprima/Jint containing this fix any soon? I want to upgrade mobile apps from Jint 2 to Jint 3 beta and run some major manual testing on physical devices, so I either wait for the next release or use downgraded packages. Wdyt?

lahma commented 1 year ago

@h15ter did you have time to try this new version? MyGet feed has every version built from main. Just to be sure that this actually now works as expected.

lahma commented 1 year ago

The dependency chain (esprima+jint) has been released to NuGet with the latest bits.

munichmule commented 1 year ago

Awesome! We didn't run the full test yet, but it looks like the original problem has gone: scripts can be parsed and executed in AOT mode with the latest version. Thank you!

lahma commented 1 year ago

Great to hear, thanks for confirming 👍🏻