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

#1573 - added convenience API #1574

Merged
merged 1 commit into from Oct 16, 2023

Conversation

gentledepp
Copy link
Contributor

Please do not merge - test fails!

public void Setup()
{
engine = new Engine();
parsedScript = new Esprima.JavaScriptParser().ParseScript(sourceCode);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should use Engine.PrepareScript(sourceCode)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aah... interesting.
Is this only for performance tests, or should we also use Engine.PrepareScript(xxx) instead of new Esprima.JavaScriptParser().ParseScript(sourceCode); in our production code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done - does not seem to make too much of a difference though:

Method Mean Error StdDev Gen0 Gen1 Allocated
ReusingEngine 3,921.6 ns 77.99 ns 152.12 ns 0.5417 - 3.34 KB
NewEngineInstance 10,940.6 ns 216.49 ns 361.71 ns 2.7618 0.1373 16.93 KB
ShadowRealm 9,524.8 ns 189.78 ns 454.70 ns 2.2888 0.1526 14.06 KB
ReusingEngine_ParsedScript 911.0 ns 17.50 ns 15.52 ns 0.3204 0.0019 1.97 KB
NewEngineInstance_ParsedScript 5,345.5 ns 77.00 ns 94.57 ns 2.5253 0.1297 15.51 KB
ShadowRealm_ParsedScript 4,677.5 ns 71.50 ns 63.38 ns 2.0599 0.1373 12.64 KB

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should always prefer Engine.PrepareScript() as it's the path that will get optimizations if they are possible. When script goes through the helper we can be assured that we can take our time to analyze more try to pre-calculate things. Otherwise Jint expects that this script might not be run multiple times (it doesn't know).

I recently pushed engine construction optimizations to main so you might want to rebase against it and see if things are better. NewEngineInstance cases should be faster at least.

@lahma
Copy link
Collaborator

lahma commented Oct 11, 2023

@gentledepp may I inquire the status of this PR, there's a lot of great work but still a draft and could probably land main after some tweaks?

@gentledepp
Copy link
Contributor Author

@lahma sure I will check tomorrow.

Some tweaks === rebase && tests Green
?

@lahma
Copy link
Collaborator

lahma commented Oct 11, 2023

Some tweaks === rebase && tests Green ?

Exactly, sorry for the ambiguity. The API should be sound as it reflects non-shadow version.

@gentledepp
Copy link
Contributor Author

So... did a rebase.
Here are the updated benchmark results:

BenchmarkDotNet v0.13.7, Windows 11 (10.0.22621.2428/22H2/2022Update/SunValley2)
Intel Core i7-10750H CPU 2.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET SDK 8.0.100-preview.7.23376.3
[Host] : .NET 6.0.23 (6.0.2323.48002), X64 RyuJIT AVX2
DefaultJob : .NET 6.0.23 (6.0.2323.48002), X64 RyuJIT AVX2

Method Mean Error StdDev Gen0 Gen1 Allocated
ReusingEngine 3,612.9 ns 43.64 ns 36.44 ns 0.5417 - 3.34 KB
NewEngineInstance 8,726.6 ns 168.19 ns 241.21 ns 2.6550 0.1373 16.28 KB
ShadowRealm 8,123.6 ns 142.88 ns 200.30 ns 2.2736 0.1526 13.98 KB
ReusingEngine_ParsedScript 954.4 ns 18.56 ns 24.78 ns 0.3204 0.0019 1.97 KB
NewEngineInstance_ParsedScript 4,365.1 ns 77.92 ns 80.02 ns 2.4185 0.1144 14.86 KB
ShadowRealm_ParsedScript 4,220.9 ns 93.78 ns 272.08 ns 2.0485 0.1335 12.56 KB

@gentledepp
Copy link
Contributor Author

Also, 19 tests still fail, but these are the same 19 tests that also fail on main
image

@lahma
Copy link
Collaborator

lahma commented Oct 13, 2023

Are you happy with the code, should we merge?

@gentledepp
Copy link
Contributor Author

gentledepp commented Oct 16, 2023

Yes, I am quite happy with it. But I am open for suggestions.
We are still running JINT 2.x so we are not in a hurry ;-)

The only thing that came to my mind was that it might be useful to have a common interface for the Engine and the ShadowRealm as far as the convenience API goes.
Because I actually do not care what it is that evaluates my code.
But this is just a feeling - and I have no concrete scenario nor a good name for it. So I skipped this.
Also: One can still create a wrapper if that'd be required.

(Sidenote: We actually tried to migrate to 3.x but experienced evaluation timeouts since then, so we had to switch back. Cannot wait until it is actually released :-D)

@lahma
Copy link
Collaborator

lahma commented Oct 16, 2023

OK, I think we can bring this in, thanks for iterating.

(Sidenote: We actually tried to migrate to 3.x but experienced evaluation timeouts since then, so we had to switch back. Cannot wait until it is actually released :-D)

Probably won't be fixed by release if there's still problems that we don't know of...

@lahma lahma marked this pull request as ready for review October 16, 2023 16:44
@lahma lahma merged commit f4e0ce3 into sebastienros:main Oct 16, 2023
3 checks passed
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

2 participants