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

fixes #615 improve engine creation performance #625

Merged
merged 11 commits into from
May 17, 2019

Conversation

lahma
Copy link
Collaborator

@lahma lahma commented May 17, 2019

  • we already have lazy initialization support for arguments instance, so I derived a solution where objects are initialized lazily
  • first access to features initializes objects, so if you don't use Map, it won't allocate or initialize
  • no performance impact on earlier benchmarks

Before

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
EngineBenchmark 56.49 us 0.0473 us 0.0442 us 26.9775 0.1221 - 110.51 KB
EvaluationBenchmark 791.4 us 0.7033 us 0.6578 us 138.6719 - - 570.16 KB
UncacheableExpressionsBenchmark 225.8 ms 0.1986 ms 0.1858 ms 30333.3333 333.3333 - 122.57 MB

After

  • memory allocations reduced 92%
  • minimal initialization and simple script run reduced 95%
Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
EngineBenchmark 2.745 us 0.0093 us 0.0078 us 2.1248 - - 8.72 KB
EvaluationBenchmark 796.4 us 1.711 us 1.601 us 138.6719 - - 570.16 KB
UncacheableExpressionsBenchmark 223.4 ms 0.1680 ms 0.1571 ms 30333.3333 333.3333 - 122.57 MB

@sebastienros
Copy link
Owner

I think it should be a huge gain for ravendb also. Will benchmark it on Orchard tomorrow, because we don't even use all these objects.

@lahma lahma force-pushed the perf/faster-engine-construction branch from 7eb11d9 to 1b03d8c Compare May 17, 2019 06:45
@sebastienros sebastienros merged commit 5e0b3bb into sebastienros:dev May 17, 2019
@lahma lahma deleted the perf/faster-engine-construction branch May 17, 2019 19:23
@lahma
Copy link
Collaborator Author

lahma commented Aug 14, 2019

@sebastienros did you ever find the time to compare performance in Orchard?

@sebastienros
Copy link
Owner

I remember I checked and it was faster but don't recall the numbers.

@lahma
Copy link
Collaborator Author

lahma commented Aug 14, 2019

Great, thanks for the update 🙂

@sebastienros
Copy link
Owner

if you like perf, there is so much to do in Orchard. It's allocating like crazy, which is not surprising because it does stuff ;) but which also means there are many ways to improve it.

@lahma
Copy link
Collaborator Author

lahma commented Aug 14, 2019

Maybe at some point, probably requires quite a lot studying to understand hot paths and important scenarios...

@@ -59,32 +80,35 @@ public sealed class Engine
internal readonly ArgumentsInstancePool _argumentsInstancePool;
internal readonly JsValueArrayPool _jsValueArrayPool;

public ITypeConverter ClrTypeConverter;
public readonly ITypeConverter ClrTypeConverter;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this done? It is now no longer possible to override with a custom ITypeConverter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just an oversight. Maybe submit a PR with a test case that protects from happening again?

Copy link
Contributor

Choose a reason for hiding this comment

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

See #654 for pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants